-
Notifications
You must be signed in to change notification settings - Fork 235
fix(tooltip): remove width override [#5612] [SWC-994] #5653
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
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
0612da8 to
480795f
Compare
72e0697 to
8e6698e
Compare
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 is a great start of how/why we should do this since consolidating styles fixes not only the max-width bug, but also brings back some of the placement options that didn't work before (top-start, top-end, bottom-start, bottom-end).
I also noticed looking over this that I'm not sure that the right-start, right-end, left-start, and left-end placements are doing anything. I didn't see styles for them. There are also some placement options available in CSS that we don't use here, like right top, right bottom, end top, end bottom, etc. Something we could add later on if we wanted to!
| :host([placement="bottom-start"]) #tip, | ||
| :host([placement="top-start"]) #tip { | ||
| inset-inline: var(--mod-tooltip-pointer-corner-spacing, var(--spectrum-tooltip-pointer-corner-spacing)) auto; | ||
| } |
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.
Looks like top-start, top-end, bottom-start, and bottom-end all aren't working as expected on the main branch, here they're fixed so that they actually work now 🎉
| :host([placement*="right"]) #tip { | ||
| clip-path: polygon(calc(100% - var(--mod-tooltip-tip-height-percentage, var(--spectrum-tooltip-tip-height-percentage))) 50%, calc(100% + var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))) 100%, calc(100% + var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))) 0); | ||
| inset-inline: calc(var(--mod-tooltip-tip-block-size, var(--spectrum-tooltip-tip-block-size)) * -2) 100%; | ||
| } | ||
|
|
||
| :host([placement*="left"]) #tooltip #tip { | ||
| :host([placement*="right"][dir="rtl"]) #tip { | ||
| clip-path: polygon(var(--mod-tooltip-tip-height-percentage, var(--spectrum-tooltip-tip-height-percentage)) 50%, calc(100% + var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))) 100%, calc(100% + var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))) 0); | ||
| left: auto; | ||
| right: 100%; | ||
| } |
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.
I think the clip-paths here and also for the corresponding [placement*="left"] may be the same, it might be possible to eliminate the rtl clip-path. I'm less sure about inset-inline vs. left/right but if we were to do this for real this seems like a good opportunity to start using logical properties.
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.
I believe we used left and right here with the intention that we didn't want it to respond to changes in language direction. Our docs are meant to guide users to start and end as the preferred but for use-cases where they want positioning fixed regardless of language, these are available.
5d4e947 to
a438460
Compare
a438460 to
b3abd01
Compare
Description
Consolidated all available CSS assets into a single
tooltip.cssfile and removed duplicate styles from the shipped asset. Restructured similar styles together and removed unused styles to improve maintainability and reduce bundle size.Note: Added a TODO comment on line 135 for the
#tip[style]selector rule that needs additional documentation to explain its purpose and usage.Motivation and context
The tooltip component had multiple CSS files that contained duplicate styles and unused code. This consolidation:
Related issue(s)
Screenshots (if appropriate)
N/A - CSS consolidation only
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Basic tooltip functionality with all variants
Tooltip placement and positioning
Tooltip animations and transitions
Tooltip in RTL (right-to-left) layout
dir="rtl")High contrast mode support
Device review