feat(link): new typography tokens, focus color and design, WHCM focus state#3570
Conversation
🦋 Changeset detectedLatest commit: b24cbc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
9a2a1e9 to
773de88
Compare
File metricsSummaryTotal size: 1.37 MB*
link
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3570--spectrum-css.netlify.app |
d5e72d4 to
0f14273
Compare
8bcd07d to
d3a605e
Compare
3499231 to
ad861d0
Compare
rise-erpelding
left a comment
There was a problem hiding this comment.
This was such a nicer-sized migration to review compared to some of the ones we've had lately! 😸
The primary, secondary, and static colors are on point and looking good, same for standalone default and standalone quiet! The main recommendation that I'd make here is to ensure that we can see the inline variant with Storybook controls, tests, and with a story of its own on the Docs page. It looks like the only real change there is maybe just the font weight? It seems like there's a clear differentiation between standalone and inline links in S2, so it's probably worth adding something more in the documentation. For instance, we probably want to demonstrate it without filler text, and even if we don't name the variant standalone in CSS (IMO it's fine if the default is standalone), we should probably note somewhere on the docs that standalone is the default.
The changelog says that inline quiet links were removed for accessibility reasons, that's probably a good thing we can integrate into our updated docs page for link. Ideally, we can add this into the JSDoc comments somewhere, and hopefully we'll have Storybook picking those up and putting them back into story descriptions again soon!
Last thing: I'm also wondering if it's possible to stack variants within a story? Like put primary and primary (quiet) in a single frame? I don't know if that was a deliberate decision made when we did the docs site to storybook migration for Link or whether the migration happened before we started doing that, but it might be nice to make the docs page a little bit neater. Up to you.
5334465 to
c8ecf03
Compare
…ub.com/adobe/spectrum-css into aramos-adobe/css1019-link-s2-migration
rise-erpelding
left a comment
There was a problem hiding this comment.
I think this is almost there!
There are a few, mostly documentation things that I think could make this even more polished - some of these might also be in the individual comments, but I'm duplicating them here because occasionally those comments get lost:
- Docs page:
- We could say something about the standalone link being the default variant vs. the inline variant.
- Rename the Multiline story to include something about focus state in the title (I don't feel super strongly about this one since you talk about it in the JSDoc comment, so I'll leave it to your discretion).
- Consider changing the wrapping element for TemplateWithFillerText and MultilineText to a
<p>instead of<div> - Mention the classes being used for variants in sentences.
- Storybook controls - Remove the ability to turn on both inline and quiet controls.
- CSS - We should change the font-family to the font-stack.
- Changeset - Add a bit more about what's changing from S1 to S2 so it reads nicely in the changelog.
…ub.com/adobe/spectrum-css into aramos-adobe/css1019-link-s2-migration
castastrophe
left a comment
There was a problem hiding this comment.
Looks great! I left some questions, some suggestions, some ideas, but all of this could go in as-is if needed!
Link S2 migration
New tokens
Color
spectrum-link-focus-indicator-colorspectrum-link-focus-indicator-thicknessspectrum-link-focus-indicator-gapspectrum-link-corner-radiusTypography
spectrum-link-line-heightspectrum-link-line-height-cjk-100spectrum-link-font-sizespectrum-link-font-stylespectrum-link-font-weightspectrum-link-text-underline-thicknessspectrum-link-text-underline-gapHow 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
Regression testing
Validate:
Screenshots
To-do list