-
Notifications
You must be signed in to change notification settings - Fork 201
feat(toast): S2 migration #3643
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
🦋 Changeset detectedLatest commit: 1641138 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
🚀 Deployed on https://pr-3643--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.38 MB*
toast
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
3a3e923
to
1027d4e
Compare
3942a21
to
61add5d
Compare
7f16d35
to
4a670f9
Compare
d6f0671
to
6c327b2
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.
Another old PR, resurrected! I had some comments that I couldn't find a good place to put inline:
-
I noticed this at line 81 of the CSS:
color: var(--mod-toast-background-color-default, var(--spectrum-toast-background-color-default));
. Shouldcolor
be set to the--spectrum-toast-text-and-icon-color
(which is white) instead? It's set later in the file for.spectrum-Toast-content
and.spectrum-Toast-typeIcon
, but I'm curious why it's set to the background color. If we set the font/icon color in.spectrum-Toast
, will it cascade correctly? And then we can delete all of those repetitive color declarations for negative, -typeIcon, and content. Quickly, testing that, I think it works. -
In the stories file, could we revisit the documentation? Specifically, I think the
Action
story is worded oddly. I misunderstood it a few times in a few places while reading, partially because the accepted button variant is so specific. Maybe we use something like:
"A toast can offer up to one user action using a secondary static white button, with the outline treatment. That button's label should be kept concise, and it should only be used when there’s a direct action available that is related to the toast text."
accc37b
to
5514df5
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.
I think I have more changes to request, but I didn't want to hold up the comments I did have. I'll be back to see if I can work out the calcs/max functions!
80ce997
to
f31071a
Compare
f31071a
to
b5a75cf
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.
I think this is looking great! Thanks for getting back to me on the --noButton
question I had.
Also- feel free to reword my WIP commits! I wouldn't want them to accidentally get merged or something with my very helpful "style wip" message 😆
294aeb8
to
549bc5d
Compare
549bc5d
to
288e469
Compare
288e469
to
1641138
Compare
Closing out + merging with 2 existing reviewers.
Description
CSS-619
S2 toast migration
This migrates the
toast
component to S2. Custom properties have been remapped per the design spec. Mods have been added and placeholder icons have been updated from the new workflow icons.Tokens
Mods
The following mods have been added.
--mod-toast-font-family
--mod-toast-font-style
--mod-toast-icon-block-size
--mod-toast-spacing-action-button-to-close
--mod-toast-spacing-bottom-edge-to-close-button
--mod-toast-spacing-close-button-to-edge
--mod-toast-spacing-text-to-close-button
--mod-toast-spacing-top-edge-to-close-button
Passthroughs
The following passthrough has been added.
--mod-closebutton-icon-color-default
Validation steps
toast
component and verify no regressions have occurred.Screenshots
To-do list