feat(dialog): s2 standard dialog migration #2860
Conversation
🦋 Changeset detectedLatest commit: 3c34cba The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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.29 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsavatar
buttongroup
dialog
modal
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-2860--spectrum-css.netlify.app |
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
6f90f2f
to
1e8b017
Compare
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
1e8b017
to
596d2f6
Compare
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
3 times, most recently
from
abaf623
to
4f08fa7
Compare
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
49d137e
to
a57836c
Compare
marissahuysentruyt
requested review from
rise-erpelding,
pfulton,
jawinn and
castastrophe
There was a problem hiding this comment.
Hey, this component is a beast and you did a nice job! There are so many changes for S2 and it matches the spec nicely for most typical cases! I'm going to go ahead and submit these initial thoughts so I'm not sitting on this for too long but I may come back and add more comments later!
Wrapping
I didn't dig into the minmax function that you added that you commented on, but I did note that there were some funky formatting/wrapping issues that may or may not be fixed by adjusting that grid-template-columns:
- Adding more "Additional header content" will stretch this container and push the heading text so that it wraps and eventually gets pushed out of the dialog on the left, that's probably not expected behavior. That's probably not expected behavior. On the plus side, I can see that the heading text is wrapping nicely!
- In the footer, adding too much footer content also does a weird thing where it doesn't wrap and pushes the action buttons out of the dialog on the right side (although the footer text does eventually wrap, which is good!), that's probably not ideal either. It might help to have some max-width values/tokens for these two content areas.
Fullscreen/Fullscreen Takeover Mode
- I called out the close button positioning in a comment, I think. It ends up being really funky if the close button is added, is the close button allowed to be used for fullscreen mode? Also, the close button is appearing in addition to the action buttons, and I had thought this should be an either/or situation (so correct me if I'm wrong!).
- The footer content disappears in fullscreen, is that expected?
- I made this callout in a comment but I'm going to leave it here too so it doesn't get forgotten: do we definitely want border-radius on the dialog for the fullscreen takeover mode?
- I cannot remove fullscreen/fullscreen takeover as an option once I select it, so I can't go back to the modal that it starts on without removing the parameter from the url. It would be nice to have this be an option so that I could go back to it after looking at fullscreen.
- The hero image control doesn't work in Storybook if I'm on fullscreen mode. I'm not sure whether or not this is intentional, but if fullscreen dialogs are not meant to have a hero image, we should remove controls for those if possible
Tray
I captured it with two dialogs but I just looked again and didn't see the issue 🤷♀️ But I also just saw it when I checked main so I don't think this is something you caused with this work.
|
@rise-erpelding thanks for all of this feedback!
I left a comment about all of these! They're things I've asked about, but I don't think have been decided on yet for S2. I have a Jira ticket to follow up on the 3 questions that still remain for the fullscreen layouts. (this is the hero & dismissible control removals: 0bfa1a8) for fullscreen layouts)
I agree. I ended up having to alter the template a little more and sort of rework some arguments, so I'd love for you to try to break things!! I think I got that figured out: 0bfa1a8
That was intentional on my part. Unfortunately, it seems like you found half of the work and I didn't fix the controls 😬 They should be removed now: 94220e6
Was there something going on with Tray? I know it uses a dialog, but that wasn't one of the components I intentionally touched. As for the header and footer overflow and grid placement- still working on it. 👍 EDIT: I moved the Chromatic testing templates to |
- adds is-hidden-story to replicate what is on main and remove most stories from the sidebar - adds some comments and TODOs giving context to the fullscreen and fullscreenTakeover stories being hidden for now while guidance is created.
- add typography to peerDependencies - remove divider from peerDependencies and peerDependenciesMeta - adds closebutton and typography to peerDependenciesMeta
- adds some opportunities for mod properties - fixes some spacing between heading & optional header content - fixes some spacing between footer content and button group - updates the changeset with new mods
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
88db1a4
to
4168200
Compare
components/dialog/index.css
Outdated
| border-radius: var(--mod-standard-dialog-border-radius, var(--spectrum-standard-dialog-border-radius)); | ||
| overflow: hidden; | ||
|
|
||
| .spectrum--large & { |
There was a problem hiding this comment.
Can .spectrum--large be converted by SWC? I could be wrong, but I thought that part of the reason we have custom-vars in the tokens package was because they had trouble with us using large platform scale and light/dark classes like this.
There was a problem hiding this comment.
Thanks for calling this out. Glad we got confirmation from Cassondra that we cannot do this and I can correct it before it goes out to SWC.
For posterity, we cannot nest platform styles like this:
because the individual web components are not aware of the context they exist inside. Namely, we can't rely on the element having a ".spectrum--large" class to look up to. We can add...the default value to the custom medium styles (
custom-medium-vars) and the alt to custom large (custom-large-vars)and it should toggle accurately
Doing so ☝️ allows us to access it like a global variable.
| </span> | ||
| `, | ||
| () => html` | ||
| <span class="${rootClass}-heading--noHeader"></span> |
There was a problem hiding this comment.
I'm seeing some extra space appearing when there is no "additional header content", as a result of the gap property and this empty element being there. When I remove the element, it seems to look fine. Do we need this empty element and this --noHeader class? I remember there were questions about the purpose of this class a few weeks back; did you find out more about it? I'm trying to find any other visual difference when it is used vs when it isn't; I'm not seeing a difference when disabling some of the margin-inline-end: 0 styles connected to this class.
| control: { type: "text" }, | ||
| if: { arg: "hasFooter", truthy: true }, | ||
| }, | ||
| hasCheckbox: { |
There was a problem hiding this comment.
I'm not seeing the checkbox appear for fullscreen and fullscreenTakeover layout options. Or is this something where we need Storybook to allow multiple conditional controls in order to only show it for the default layout?
There was a problem hiding this comment.
For right now, the checkbox and/or footer text is not supported in the fullscreen or fullscreenTakeover stories. Rise and I did decide we'd need multiple conditionals to only show it. Maybe we can talk more about this during our dev sync?
There was a problem hiding this comment.
Also- I may have moved that work to the other dialog branch. I'd have to check on it though.
| @@ -46,39 +116,186 @@ export default { | |||
| }, | |||
| control: "boolean", | |||
| }, | |||
| heroImageUrl: { | |||
There was a problem hiding this comment.
It'd be nice if I could view the hero image from the controls, without having to upload an image. Are we missing the hasHeroImage control? I see it referenced once in the Docs story.
There was a problem hiding this comment.
Let me know if this is close to what you were thinking! 9d8f881
components/dialog/index.css
Outdated
| min-inline-size: var(--mod-standard-dialog-min-inline-size, var(--spectrum-standard-dialog-min-inline-size)); | ||
| max-inline-size: 100%; | ||
| max-block-size: inherit; | ||
| background-color: var(--spectrum-standard-dialog-background-color); |
There was a problem hiding this comment.
Previously the background color was being set on Modal that wraps the Dialog, so now that we're setting the background color here on the Dialog, there is some antialiasing bleed through happening on the corners since both now have background colors on top of each other:
If you set --mod-modal-background-color: hotpink; you can see the issue better:
It's kind of a CSS rendering quirk, even though these things have the same border-radius; I encountered something similar when working on color slider.
I'm not sure why Modal has the background (or why Modal is separate from Dialog to begin with), but it seems like we'll need to work toward making its background transparent? That would affect alert dialog and tray too, so I'm not sure about the best course. A stop-gap here might be to use --mod-modal-background-color: transparent; in this dialog template, but we can't really build it into Dialog's CSS since Modal is the parent. This could be something to talk about at team meeting.
Also, do we need a mod for this?
set --spectrum-standard-dialog-spacing-edge-to-content tokens for desktop and mobile
- adds a hasHeroImage control - refactors template to check for hasHeroImage - sets a default image for heroImageUrl
- also adds a comment about multiple conditionals for argTypes
- calls for --spectrum-standard-dialog-spacing-edge-to-content - removes .spectrum--large nesting - fixes dismissible grid spacing between additional header content and close button
because modal was originally setting the background color of dialogs, there was some 'antialiasing bleed through' behind the dialog. By adding customStyles to Modal, we can set --mod-modal-background-color to transparent and get rid of the hairline color (especially noticeable behind a hero image in a dialog)
Description
This migrates the Standard Dialog to S2. 🎉 The default dialog story is called "Standard Dialog." New tokens were utilized to match the design specs. The template underwent significant changes (first uncovered in #2833) to accommodate missing elements such as optional header content, optional footer content, hero/cover images, and the button group. There is also a new option for the footer content, where a checkbox and text can be present, or just the text itself.
Documentation has been updated and the divider story was removed since dividers are no longer supported in dialog components in S2.
This PR pulls in certain features from
mainin preparation for whenspectrum-twomerges. That includes someimportstatements that are commented out and thedialog.test.jsfile. While the test file doesn't do anything on this branch at this time, it has been updated. Dialogs with dividers are no longer supported, so that test case has been removed.Because the dialog was renamed to "standard dialog" and the divider component was removed, mod properties are either "new," have been renamed to reflect the "standard dialog" language, or removed:
New Mods
--mod-standard-dialog-spacing-title-to-header-content--mod-standard-dialog-spacing-title-to-description--mod-standard-dialog-header-content-font-sizeRenamed Mods
--mod-dialog-confirm-small-width>--mod-standard-dialog-max-width-small--mod-dialog-confirm-medium-width>--mod-standard-dialog-max-width--mod-dialog-confirm-large-width>--mod-standard-dialog-max-width-large--mod-dialog-confirm-border-radius>--mod-standard-dialog-border-radius--mod-dialog-confirm-description-text-size>--mod-standard-dialog-description-font-size--mod-dialog-confirm-description-text-line-height>--mod-standard-dialog-description-line-height--mod-dialog-confirm-description-text-color>--mod-standard-dialog-description-text-color--mod-dialog-confirm-description-font-weight>--mod-standard-dialog-description-font-weight--mod-dialog-heading-font-weight>--mod-standard-dialog-heading-font-weight--mod-dialog-confirm-title-text-line-height>--mod-standard-dialog-heading-line-height--mod-dialog-confirm-title-text-color>--mod-standard-dialog-heading-text-color--mod-dialog-confirm-title-text-size>--mod-standard-dialog-heading-font-size--mod-dialog-confirm-hero-height>--mod-standard-dialog-hero-block-size--mod-dialog-min-inline-size>--mod-standard-dialog-min-inline-size--mod-dialog-confirm-padding-grid>--mod-standard-dialog-spacing-grid-padding--mod-dialog-confirm-footer-padding-top>--mod-standard-dialog-spacing-description-to-footer--mod-dialog-confirm-close-button-padding>--mod-standard-dialog-spacing-edge-to-close-button--mod-dialog-confirm-gap-size>--mod-standard-dialog-spacing-footer-to-button-groupRemoved Mods
--mod-dialog-confirm-buttongroup-padding-top--mod-dialog-confirm-close-button-size--mod-dialog-confirm-description-margin--mod-dialog-confirm-description-padding--mod-dialog-confirm-divider-block-spacing-end--mod-dialog-confirm-divider-block-spacing-start--mod-dialog-confirm-divider-heightNew mod for button group:
--mod-buttongroup-flex-wrapDesigns
S2 Standard Dialog Tokens specs
S2 / Desktop Standard Dialog
There will be subsequent work for the fullscreen and fullscreenTakeover dialogs, (draft PR: #3347).
Jira
CSS-714
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
Dividercomponent, so any JS or CSS referencing the dialog's divider has been removed from the stories andindex.cssfiles.spectrum-Dialogand.spectrum-Modalutilize the updated border-radius value (corner-radius-extra-large-default16px)--small,--mediumand--largehave been removed from the JS and CSS. Any sizing modifiers should now reflect T-shirt sizing (--sizeS,size--L), without a--sizeMas that is the default dialog size.standard-dialog-maximum-width-smallstandard-dialog-maximum-width-mediumstandard-dialog-maximum-width-largestandard-dialog-minimum-widthstandard-dialog-title-font-sizestandard-dialog-body-font-sizecorner-radius-extra-large-defaultspacing-500spacing-400spacing-300background-layer-2-color(in the designs, this is accidentally misnamed asbackground-color-layer2line-height-200dialog.test.jsfile) will include the default, with a hero image, both as non-dismissible (the default) and dismissible (isDismissible: true). The fullscreen and fullscreenTakeover are separate test templates, and do not have additional variations/test scenarios.Regression testing
Validate:
Screenshots
To-do list