Skip to content
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 our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

use popover API in Tooltip if supported #2060

Merged
merged 14 commits into from
May 23, 2024
Merged

use popover API in Tooltip if supported #2060

merged 14 commits into from
May 23, 2024

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented May 22, 2024

Changes

When supported, popover=manual will be used to show the tooltip in the top-layer. In non-supported browsers, the attribute is ignored.

In addition to the popover attribute itself, there are only two changes:

  • JS: Calling togglePopover whenever the React state changes.
    • This is guarded using ?. for supporting older browsers.
    • This also required adding a custom TS type, which can be removed in the future.
  • CSS: Hiding the tooltip when :popover-open doesn't match.

I kept the existing mechanism (Portal component, z-index property, hidden attribute), so Tooltip will continue to work in browsers that don't support popover.

Testing

All existing tests pass 馃殌

Tested manually that hover/focus behaviors are intact and that the tooltip does indeed show in the top layer (This can be verified in Chrome dev tools, for example). Also emulated older browsers by commenting out popover and testing that it works fine.

Updated unit tests to simply check for the presence of the popover attribute. I spent some time looking into adding tests for togglePopover without success, since JSDOM doesn't support it. This is not a big deal imo; the visual tests already serve as a way to ensure that the tooltip is working.

Docs

Added changesets for both CSS and React.

Updated Tooltip documentation to mention that portaling is useful only in browsers that don't support popover.

@mayank99 mayank99 self-assigned this May 22, 2024
@mayank99 mayank99 marked this pull request as ready for review May 22, 2024 18:15
@mayank99 mayank99 requested review from a team as code owners May 22, 2024 18:15
@mayank99 mayank99 requested review from r100-stack and Ben-Pusey-Bentley and removed request for a team May 22, 2024 18:15
@mayank99 mayank99 enabled auto-merge (squash) May 23, 2024 14:41
@mayank99 mayank99 merged commit ef4debf into main May 23, 2024
16 checks passed
@mayank99 mayank99 deleted the mayank/tooltip-popover branch May 23, 2024 14:48
@imodeljs-admin imodeljs-admin mentioned this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants