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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always set :absolute position on Primer::Alpha::Tooltip #1281

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

jonrohan
Copy link
Member

@jonrohan jonrohan commented Aug 5, 2022

I was working on SegmentedController and noticed a small size jump with the components. It turns out because there's a moment before the Javascript is loaded in that the Tooltip is hidden, but is taking up dom space. It isn't noticeable in our previews because we're only rendering one of the items that have a tooltip, but I disabled JS for the screenshot below and put a second button in line.

image

I think we should have a hardcoded position: absolute; css that will keep the element out of the flow even when JS fails to load. I opted for adding the system argument, but I'm open to other ways.

@changeset-bot
Copy link

changeset-bot bot commented Aug 5, 2022

🦋 Changeset detected

Latest commit: 8e0d302

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jonrohan jonrohan marked this pull request as ready for review August 5, 2022 23:18
@jonrohan jonrohan requested review from a team and keithamus August 5, 2022 23:18
@jonrohan jonrohan temporarily deployed to github-pages August 5, 2022 23:21 Inactive
Copy link
Contributor

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this is most likely because of the recent #1256.

We had to switch from using hidden to visibility:hidden; to hide the tooltip as a workaround for a bug discovered by James in screen readers. This bug is likely specific to custom element + accessibility API. We also discovered that display: none had the same issue for SR users.

visibility: hidden; let us get around the bug and helps us properly expose accessible names and descriptions but there's the caveat that it takes up space.

this change to make sure the tooltip is out of the document flow on page load should be good 👍. thank you!!

@jonrohan jonrohan merged commit 843061d into main Aug 5, 2022
@jonrohan jonrohan deleted the tooltip_position branch August 5, 2022 23:57
@primer-css primer-css mentioned this pull request Aug 6, 2022
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.

3 participants