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

fix(popover, tooltip): change display to contents #11384

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

driskull
Copy link
Member

@driskull driskull commented Jan 25, 2025

Related Issue: #11378

Summary

  • change default display on tooltip and popover from block to contents.

BEGIN_COMMIT_OVERRIDE
omitted from changelog
END_COMMIT_OVERRIDE

@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 25, 2025
@github-actions github-actions bot added the refactor Issues tied to code that needs to be significantly reworked. label Jan 25, 2025
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 25, 2025
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 27, 2025
@driskull driskull marked this pull request as ready for review January 27, 2025 16:59
Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Code changes look good! Is there a quick explainer of the pros / any cons of this change or things implementers should look out for?

@driskull
Copy link
Member Author

Here's a good explainer: https://css-tricks.com/get-ready-for-display-contents/

One answer to this is display: contents;—a magical new display value that essentially makes the container disappear, making the child elements children of the element the next level up in the DOM.

display: contents makes that the div doesn’t generate any box, so its background, border and padding are not rendered. However the inherited properties like color and font have effect on the child (span element) as expected.

This should be fine since we don't want the host of the tooltip/popover to have any dimensions or affect any parent elements dimensions.

@driskull
Copy link
Member Author

Something to watch out for is that setting a width/height on a tooltip would no longer take up any space. Although this is a pro since it doesn't have any advantage to it.

https://codepen.io/Matt-Driscoll/pen/XJroEdr?editors=1010

@driskull
Copy link
Member Author

@jcfranco can you review?

@macandcheese
Copy link
Contributor

Something to watch out for is that setting a width/height on a tooltip would no longer take up any space. Although this is a pro since it doesn't have any advantage to it.

https://codepen.io/Matt-Driscoll/pen/XJroEdr?editors=1010

Cool. Same for Popover then - folks may just need to move any set widths on Popover to a child element it sounds like.

@driskull
Copy link
Member Author

Cool. Same for Popover then - folks may just need to move any set widths on Popover to a child element it sounds like.

Yes, that is already the case for 3.0 anyway

@jcfranco
Copy link
Member

Noice!

Should this use fix (+ no changelog entry label )instead? It addresses the layout regression from #10240 by changing the display mode.

@driskull driskull changed the title refactor(popover, tooltip): display as contents fix(popover, tooltip): display as contents Jan 27, 2025
@driskull driskull added the no changelog entry Use the commit override to avoid a changelog entry label Jan 27, 2025
@driskull driskull changed the title fix(popover, tooltip): display as contents fix(popover, tooltip): change display to contents Jan 27, 2025
@driskull driskull merged commit 1d57456 into dev Jan 27, 2025
25 checks passed
@driskull driskull deleted the dris0000/popover-tooltip-display-contents branch January 27, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog entry Use the commit override to avoid a changelog entry pr ready for visual snapshots Adding this label will run visual snapshot testing. refactor Issues tied to code that needs to be significantly reworked.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants