-
Notifications
You must be signed in to change notification settings - Fork 201
feat(alert dialog): S2 migration #3725
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
base: spectrum-two
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: bd46f2b 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-3725--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.41 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsalertdialog
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
393e919
to
7a4d68f
Compare
f48db01
to
178a298
Compare
178a298
to
2f040e0
Compare
68ce9b0
to
879e039
Compare
939b11e
to
30c40a0
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.
Sorry it took me longer than I meant to get a review done! I do have some questions for you, and found a few small changes.
- Tokens are looking really great. The only one I didn't see in the CSS was
alert-dialog-top-to-alert-icon
. I'm assuming we've left it out because we have a flex/align-items center now on that heading container, or is there a reason?
And finally- There's a customModalStyles
arg being passed to the Template()
function. Is that necessary? That wasn't anything you added, I just happened to notice it. Feel free to keep it if we don't really know what it does!
.spectrum-AlertDialog-buttongroup { | ||
display: flex; | ||
justify-content: end; | ||
} | ||
|
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.
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.
Hmm, these styles result in the vertical button group being right aligned within the alert — with those removed they're still displayed vertically but are left-aligned within the dialog. ✨
components/alertdialog/index.css
Outdated
/* @TODO: update to `--spectrum-alert-dialog-body-font-size` when available */ | ||
font-size: var(--mod-alert-dialog-description-size, var(--spectrum-alert-dialog-description-size)); |
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 related to the title font size token I just mentioned as well. Maybe it's worth an ask, just to make sure.
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.
Good catch! We had --spectrum-alert-dialog-description-font-size
available, so I aliased it...
--spectrum-alert-dialog-description-content-size: var(--spectrum-alert-dialog-description-font-size);
...(--spectrum-alert-dialog-description-size
is the name of another token) and applied it:
font-size: var(--mod-alert-dialog-description-content-size, var(--spectrum-alert-dialog-description-content-size));
✨
00ef68d
to
829c762
Compare
… to fix case where header + footer wouldn't fill width appropriately with short content (CSS-1133)
829c762
to
bd46f2b
Compare
Description
CSS-1017
+CSS-1133
This migrates the
alert dialog
component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification.The icon in the dialog header has been moved from the right side to the left.
The dialog divider has been removed.
Primary button style has been updated to filled.
Multiple variants are now supported:
AlertDiamond
icon)AlertTriangle
icon)Dialog buttons may be displayed either horizontally or vertically.
Removed custom properties
--spectrum-alert-dialog-body-font-size
--spectrum-alert-dialog-description-to-buttons
--spectrum-alert-dialog-divider-to-description
--spectrum-alert-dialog-padding
--spectrum-alert-dialog-title-size
--spectrum-alert-dialog-title-to-divider
--spectrum-alert-dialog-title-to-icon
New custom properties
--spectrum-alert-dialog-background-color
--spectrum-alert-dialog-corner-radius
--spectrum-alert-dialog-description-to-button-group
--spectrum-alert-dialog-edge-to-content
--spectrum-alert-dialog-minimum-title-to-icon
--spectrum-alert-dialog-title-to-description
Removed mods
--mod-alert-dialog-body-font-size
--mod-alert-dialog-description-to-buttons
--mod-alert-dialog-divider-to-description
--mod-alert-dialog-padding
--mod-alert-dialog-title-to-divider
--mod-alert-dialog-title-to-icon
New mods
--mod-alert-dialog-background-color
--mod-alert-dialog-corner-radius
--mod-alert-dialog-description-size
--mod-alert-dialog-description-to-button-group
--mod-alert-dialog-edge-to-content
--mod-alert-dialog-minimum-title-to-icon
--mod-alert-dialog-title-to-description
How 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