- Notifications
You must be signed in to change notification settings - Fork1k
refactor: replace other deprecated Popovers which open on hover#19753
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
AgentOutdatedTooltip
IdpOrgSyncPageView, TableColumnHelpTooltip, TemplateInsightsPage
LegacyGroupSyncHeader
OrganizationBreadcrumb
This reverts commitc93e092.
| awaitwaitFor(async()=> | ||
| expect(awaitscreen.findByText(/v2\.\d+\.\d+/i)).toBeInTheDocument(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This fixes a test flake that occurred when#19709 was merged:https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=68d4285c06f5a4e63dfb7a42
This test failed in this branch too:https://www.chromatic.com/test?appId=624de63c6aacee003aa84340&id=68d59529f0a7d3010a53e7f2 but we're good now 👍🏽
| <MUITooltiptitle="This user has this role for all organizations."> | ||
| <span>{role.display_name||role.name}*</span> | ||
| </MUITooltip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oof. could we switch this to our ownTooltip component while we're at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The MUI tooltip component is used in 37 files including this one. I think I could reasonably knock out that migration in a single PR (separate from this one). Mind if we defer until then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
good plan!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🙌🏽 that's now being tracked in this issue, for future reference:#19974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
noticed one thing on a second pass, but otherwise looks good!
site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/OrganizationPills.tsx OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
10d41b3 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
closes#19397
dependency:#19709
#19635 covered a large chunk of deprecated
Popovers which open on hover, specifically the ones which instantiatedHelpTooltip. This PR replaces all of the non-HelpTooltiphover-triggered popovers, and removes the deprecated popover component :)