feat(tooltip): S2 migration#2743
Conversation
🦋 Changeset detectedLatest commit: 47676be The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
File metricsSummaryTotal size: 4.22 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailstooltip
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-2743--spectrum-css.netlify.app |
mdt2
force-pushed
the
mdt2/css-717-s2-tooltip
branch
from
6047fc3 to
20178ac
Compare
| background-color: var(--highcontrast-tooltip-background-color-default, var(--mod-tooltip-background-color-default, var(--spectrum-tooltip-background-color-default))); | ||
|
|
||
| /* Default: Pointing down ▽ */ | ||
| clip-path: polygon(0 calc(0% - var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))), 100% var(--mod-tooltip-tip-height-percentage, var(--spectrum-tooltip-tip-height-percentage)), calc(0% - var(--mod-tooltip-tip-antialiasing-inset, var(--spectrum-tooltip-tip-antialiasing-inset))) 100%); |
There was a problem hiding this comment.
The problem with the previous clip-path was that I couldn't apply a border-radius to it when there was a 50% point. So I adjusted the sizing of the square (see above) and the clip-path points to be able to apply the new border-radius to the tip.
This change also has a little bonus that we don't have to redefine the clip-path for the different placements, we just have to adjust the transform: rotate() value ✨
mdt2
force-pushed
the
mdt2/css-717-s2-tooltip
branch
from
855edd3 to
f46b764
Compare
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
This looks great and I really appreciated the clear validation instructions! All of those things look good and I'm happy to approve, but I have some outstanding questions, one above about the tip size being just slightly off the design for large platform scale, but I also noticed if I look closely at the tip (I changed the color to make it easy to see--and this is Chrome which is more obvious than Firefox) it almost looks like the rotation is slightly off (I think maybe by 1 degree?), which is weird because why would you need to rotate it -46deg instead of -45deg? 🤷♀️ Totally something we could explore more or talk about or reason through in case there's some weird logical reason for it!
jawinn
left a comment
jawinn
left a comment
There was a problem hiding this comment.
One thing I noticed was that the tip looks a little overlapped in high contrast mode. Is that something that is possible to adjust?
|
@rise-erpelding great callout about the strange rotation. This was caused by trying to account for antialiasing in the clip-path. When I moved that to the positioning properties instead, this resolved 🎉 |
rise-erpelding
left a comment
rise-erpelding
left a comment
There was a problem hiding this comment.
I just took a quick look at this one again and it looks like the tip issues previously noted are sorted out now! Nice work! If there are any further adjustments based on design feedback, I'm happy to take another look, but for now, I'm giving it my approval!
rise-erpelding
force-pushed
the
mdt2/css-717-s2-tooltip
branch
3 times, most recently
from
834edcf to
0135f5b
Compare
| <div> | ||
| ${Template({ | ||
| ...args, | ||
| placement: option, | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
Wrapping the Template in a <div> solves the issue that was happening here where the Tooltip starts expanding to fill the container:
rise-erpelding
force-pushed
the
mdt2/css-717-s2-tooltip
branch
from
0135f5b to
488e8ba
Compare
| --spectrum-tooltip-line-height: var(--spectrum-line-height-100); | ||
|
|
||
| /* @todo set line-height using font size specific line-height tokens when they are finalized along with the new variable font. */ | ||
| --spectrum-tooltip-line-height: 1.2; |
There was a problem hiding this comment.
This is similar to button where the line-height token causes the overall height of the tooltip to be larger than the height token, so we need to adjust the line-height manually to make it work.
marissahuysentruyt
left a comment
marissahuysentruyt
left a comment
There was a problem hiding this comment.
@rise-erpelding Thanks for taking this on! Left a few questions for you, mainly getting myself up to speed on a few of the changes.
The line-height stuff: I left a question about it, but there's a spot in the Figma file that looks like maayyybe this is what design was seeing and we're trying to fix with the hardcoded 1.2. There's a little comparison chart. Is that correct?
4 participants
Description
This migration includes updated colors, rounding, a bigger tip, and the removal of variants (only neutral is available in Spectrum 2). As a result of the deprecation of variants, icons have also been removed.
The redesign of the tip, specifically the rounding, required a reworking of how we use clip-path and transform.
Some custom property mods have been removed:
--mod-tooltip-background-color-informative--mod-tooltip-background-color-negative--mod-tooltip-background-color-positive--mod-tooltip-icon-spacing-block-start--mod-tooltip-icon-spacing-inline-end--mod-tooltip-icon-spacing-inline-start--mod-tooltip-icon-widthAnd one mod has been added:
--mod-tooltip-tip-corner-radiusHow and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
site/package.jsontokens version to be>=14.0.0-next.7yarn installyarn devRegression testing
Validate:
Screenshots
To-do list