feat(toast): migrate to Spectrum 2 #2599
Draft
+4,516
−3,832
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pfulton
added
the
skip_vrt
|
🚀 Deployed on https://pr-2599--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.48 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsactionbutton
actiongroup
button
buttongroup
card
checkbox
closebutton
dial
fieldlabel
logicbutton
modal
page
picker
slider
splitview
table
toast
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
7c86724
to
0a218bc
Compare
mdt2
force-pushed
the
pfulton/s2-toast
branch
3 times, most recently
from
ea68444
to
73072f8
Compare
castastrophe
force-pushed
the
spectrum-two
branch
from
dde6c19
to
8dfa2af
Compare
jawinn
requested changes
Apr 1, 2024
There was a problem hiding this comment.
This is looking really good!
I saw a few things on the Figma that seem to be a little different than the specs, that will likely be questions for the design team. And I had a few suggestions for the docs and Storybook.
Figma comparison
- I noticed that the corner rounding looks more rounded than on the Figma. The token you are using is correct according to what the Figma spec says (corner-radius-800), but the rounding actually displayed on the Figma is less (10px vs 16px). It looks like either the rounding displayed on the Figma needs to be updated, or the token they have noted needs to change.
- Close button color: this says "white" on the Figma, but the migrated s2 close button as shown uses a different color (slightly transparent white, which appears to be the static white option). Does the Figma need to be updated here or are they asking for the color to differ from the Close button spec?
- Another suggestion to pass along to design, but I think the "Height" heading in the Figma should be called "Minimum height".
Docs
It'd be a good idea to update the migration guide and the PR description to specify which mods were renamed.
I also saw "Spacing (toast to edge)" at the bottom of the layout section in the Figma, set at spacing-300. I don't believe we have or need any CSS for this currently, but I'm wondering if we could/should document it for users somehow?
Storybook
These could easily be work for a followup PR, as it relates to the existing Storybook setup.
- Should "variant" be a visible control in Storybook, to be similar to other components? I was looking for it under Default, but it is currently hidden.
- I get that pesky "rendered fewer hooks..." error if I change the "Inline button label" text to blank. I have some preliminary work to try to resolve this error that may help solve this---I'm also curious about whether Storybook 8 will fix these sort of errors.
Includes new color values for Spectrum 2
BREAKING CHANGE: migrates `Action Group` to Spectrum 2, Removes some mod properties
BREAKING CHANGE: migrates Button Group to Spectrum 2 Also: * docs(buttongroup): expand chromatic coverage * refactor(buttongroup): remove extra css classes
BREAKING CHANGE: uses Spectrum 2 tokens
- @spectrum-css/preview@7.4.2-next.0 - @spectrum-css/actiongroup@5.0.0-next.0 - @spectrum-css/buttongroup@7.0.0-next.0 - @spectrum-css/tokens@14.0.0-next.0 - @spectrum-css/bundle-builder@6.3.1-next.0 - @spectrum-css/component-builder-simple@5.0.1-next.0 - @spectrum-css/component-builder@6.0.1-next.0
BREAKING CHANGE: migrates Close Button to Spectrum 2 Additionally: * test: increase chromatic coverage * fix(closebutton): pass staticColor as arg for SB display * chore(closebutton): remove themes dir * docs(closebutton): adds s2 migration notes * chore(closebutton): specify s2 tokens release for dependency --------- Co-authored-by: Patrick Fulton <pfulton@adobe.com>
* chore: migrate gray-50 to gray-25 Migrates any instance of `--spectrum-gray-50` to use `--spectrum-gray-25` as per the S2 migration guide * chore: migrate gray-75 to gray-50 Migrates usages of `--spectrum-gray-75` to use `--spectrum-gray-50` as per the s2 migration guide. * chore: migrate gray-100 to gray-75 Migrates usages of `--spectrum-gray-100` to use `--spectrum-gray-75` as per the s2 migration guide * chore: migrate gray-200 to gray-100 Migrates usages of `--spectrum-gray-200` to use `--spectrum-gray-100` as per the s2 migration guide * chore: migrate gray-300 to gray-200 Migrates usages of `--spectrum-gray-300` to use `spectrum-gray-200` as per the s2 migration guide * chore(infieldbutton): gray-300 to gray-200
* fix(commons): remove renamed mods marked for deprecation Remove mods that were renamed and previously marked for deprecation, and regenerate mods lists. This will help in reviewing the accuracy of other components' mods lists as they are being migrated to s2. * docs: migration guide notes for mod property deprecations Add notes to components affected by mod property changes in the commons basebutton. * docs(closebutton): updated docs for migration guide and icon size - Removes the "Icon size" variant options, as noted in the closebutton PR. Per Figma changelog "removed icon size as a property". - Updates migration guide with a more organized history. * fix(commons): remove another mod referencing global token Deprecates an additional mod name that was referencing a global token, and updates migration notes for all affected components.
BREAKING CHANGE: uses Spectrum 2 tokens Also: * feat(fieldlabel): medium as default and 100 sizing tokens - Use the correct "100" sized tokens for medium. - Make medium styles the default, to conform with existing pattern. .spectrum-FieldLabel--sizeM class removed as it is no longer necessary. * feat(fieldlabel): use correct font size tokens The font-size tokens used for large and extra large needed to be shifted up to match the s2 spec. * refactor(fieldlabel): remove deprecated mods Remove renamed mods marked as deprecated for S2 release. * docs(fieldlabel): spectrum 2 noted in migration notes Updates some docs and adds migration notes. Clarifies some docs around "left" and "right", and fixes duplicate ID warnings from examples code. * docs(fieldlabel): form - docs update and regenerate mods Add migration guide section to "Form", that currently lives within the Field label component. Regenerates mods to include the removal of deprecated renamed mods. * fix(fieldlabel): rename mod property referencing global token A mod custom property was referencing a global token instead of the component itself. Renames this and adds a note to the migration guide. * feat(fieldlabel): use correct sizing tokens Use correct tokens for some large and extra large custom properties, to matching with design. * style(fieldlabel): applying linter formatting adjustments Run linter fix on the fieldlabel component, which rearranges a few declarations. * docs(fieldlabel): document asterisk icon size Add documentation about asterisk UI icon sizes specified on the design. These sizes are already handled in the Storybook template, but were not really explained to the user. * feat(fieldlabel): black and white static color variants Add classes for static colors, as defined on the designs. And adds examples of these variants to the documentation. * build(fieldlabel): specify s2 tokens release for dependency Use Spectrum 2 version of tokens package in dependencies list.
…2559) * feat(cornerrounding): documentation, custom tokens - feat(cornerrounding): add custom token - chore(cornerrounding): adds dist - docs(cornerrounding): add file strucutre for foundations - feat(cornerrounding): override token value - Sets custom var to override token spectrum-corner-radius-full - chore(cornerrounding): add override to dist - Adds custom var to override token value to dist - chore(cornerrounding): adjust file structure - Also renames file - feat(cornerrounding): add Foundations story for corner-rounding - feat(cornerrounding): add action buttons and checkboxes - Adds action buttons and checkboxes stories to demonstrate corner rounding - docs(cornerrounding): update corner-rounding docs - better utilize storybook features - clean up checkbox and action button stories - hide stories in corner-rounding directory - improve table styling and css token use - improve corner rounding documentation - add tables for component size tokens - fix(cornerrounding): remove mdx for tables - fix(cornerrounding): use sentence case - feat(cornerrounding): use custom token on close button - docs: add alias note - docs: remove reference to non-alias tokens - feat(actionbutton): partial migration for corner rounding only - feat(checkbox): partial migration for corner rounding only - chore(closebutton): update custom var post-refactor - fix: token rebase issue - chore(actionbutton,checkbox): update package version - chore: use corner-radius alias token - chore: update tokens version - chore: revert unneeded component version change - docs: design requested updates, show token with example - docs: move tables to stories, inline spacing for tables * fix: includes error --------- Co-authored-by: Melissa Thompson <mthompson@heysparkbox.com>
castastrophe
force-pushed
the
spectrum-two
branch
from
8dfa2af
to
810fa5e
Compare
There was a problem hiding this comment.
This is great! Making sure all the spacing details for all the variants were just right must have taken a lot, and they almost all looked perfect as far as I could tell! ✨
I have a short list of questions/callouts that don't necessarily pertain to particular file changes made here:
- Josh mentioned that leaving the button text blank causes and error and doesn't remove the button, and I noticed the same. However, this isn't a change from the previous version.
- Josh also mentioned toast to edge spacing (from the outside of toast), I'm very curious about how this is generally handled or how we might handle this kind of thing.
- The icon to text spacing - this didn't seem to quite match the design, but are we waiting for the icons before it gets adjusted?
- I noticed when inspecting things in Storybook that for
.spectrum-Toast, and also the&--negative,&--info, and&--positivevariants all have a line below thebackground-colordeclaration that sets thecolorproperty to the same color as the background color, for instance:
background-color: var(--highcontrast-toast-background-color-default, var(--mod-toast-background-color-default, var(--spectrum-toast-background-color-default)));
color: var(--highcontrast-toast-background-color-default, var(--mod-toast-background-color-default, var(--spectrum-toast-background-color-default)));
This was here before but I was wondering if there was a particular reason to set the color and background-color to the same token before overriding with the text/icon colors we want? Maybe it has something to do with :focus-visible that wasn't clear to me?
| } | ||
|
|
||
| .spectrum-Toast-buttons { | ||
| padding-inline-start: var(--mod-toast-spacing-action-button-to-close-button, var(--spectrum-toast-spacing-action-button-to-close-button)); |
There was a problem hiding this comment.
I think the spacing between the text and the close button is off for this vertical variant. If the button is below the text, the space between the text and the close button should be the same as it would be if there was no button, that is, spacing-300. Unless I missed something, this is getting spacing-200 (12px padding) because the selector knows there's a button present.
There was a problem hiding this comment.
This is a great callout! TLDR; design has approved spacing-200 in this case.
Some context: Patrick and I chatted with design about this and agreed with them that the spacing will always be spacing-200 when an action button is used in Toast. This is because we can't tell using CSS and flexbox when the action button wraps to the next line, so there's not a good way (in CSS) to have spacing-300 in this case and spacing-200 if the button isn't wrapped.
BREAKING CHANGE: migrates Toast to use Spectrum2 tokens and values
|
Thanks for the reviews! In response to @jawinn's feedback:
In response to @rise-erpelding's feedback:
|
castastrophe
force-pushed
the
spectrum-two
branch
from
9b745e7
to
9c147fc
Compare
|
Those updates look good to address my suggestions for the docs and Storybook. One small thing; I noticed that the variant control was not set on the "Default" story, since Storybook defaults to undefined. Could you set the default for that control under |
castastrophe
force-pushed
the
spectrum-two
branch
3 times, most recently
from
4d9e185
to
a81c712
Compare
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
e03868f
to
b3b3aa5
Compare
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
f3dd9ff
to
9931a8e
Compare
castastrophe
force-pushed
the
spectrum-two
branch
5 times, most recently
from
6dbc850
to
ae7aedd
Compare
pfulton
force-pushed
the
spectrum-two
branch
2 times, most recently
from
e0e0fd2
to
698e904
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migration of Toast component to Spectrum 2 tokens/styles. Includes:
--mod-toast-divider-colorhas been removed. There is no divider included in this version of Toast.--mod-toast-spacing-text-and-action-button-to-dividerhas been renamed to--spectrum-toast-spacing-action-button-to-close-button--mod-toast-spacing-top-edge-to-dividerhas been renamed to--mod-toast-spacing-top-edge-to-close-button--mod-toast-spacing-bottom-edge-to-dividerhas been renamed to--mod-toast-spacing-bottom-edge-to-close-button--mod-toast-spacing-close-buttonhas been renamed to--mod-toast-spacing-close-button-to-edgeHow 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
To-do list