feat(dialog): s2 standard dialog migration #2860
Conversation
🦋 Changeset detectedLatest commit: 4168200 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.
Detailsbuttongroup
dialog
modal
* 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 |
marissahuysentruyt
changed the title
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
7e20bab
to
5c3c954
Compare
There was a problem hiding this comment.
I think this is really close! My main callouts now are:
- That spacing between the button group and the footer content seems a little large
- Mods! I think everything you've taken out is fine given that these are breaking changes, but there are some areas that we could add more for some of the things you've added. Dialog is getting more complex, so having a larger number of mods added than taken away would probably be ok.
- I'm also wondering about leveraging mods to allow consumers to make the dialogs work for them based on what kind of content they're putting in them. It's clear from the amount of work you've put in already that writing CSS where the rules fit every possible content situation is really, really difficult, and in those cases it might be better to allow for modification downstream. That would be a great thing to get another opinion besides mine on, though.
- This is hopefully a small ask, but if it's not, we can create a card for it: can we add a control for updating the description text? (Or was it hiding somewhere?) Looks like we do a similar thing in Alert Banner, Alert Dialog, and Help Text, but not Illustrated Message, and I'm not sure if that's an oversight or whether there's a specific reason for it (and if I already asked and you told me, I'm sorry in advance!).
components/dialog/index.css
Outdated
| outline: none; /* Hide focus outline */ | ||
|
|
||
| /* prevent the heading/title from moving out of the header element if additional content is long */ | ||
| min-inline-size: 40%; |
There was a problem hiding this comment.
I'm wondering if you have a situation like this where the heading text is shorter, maybe it wouldn't be so ideal to have this min-inline-size set to 40%, maybe they would want that text to be further right. Maybe we can offer a mod here in case someone wants it to be something else depending on their situation? That seems like the best way to allow for flexibility in controlling how much space the heading and content each take up.
| font-size: var(--mod-dialog-confirm-title-text-size, var(--spectrum-dialog-confirm-title-text-size)); | ||
| } | ||
| } | ||
| .spectrum-Dialog-grid { |
There was a problem hiding this comment.
Oh this looks great! I didn't see this in the specs anywhere, design is ok with this?
There was a problem hiding this comment.
This is the mobile/responsive view right? I tried not to touch too much with that media query, besides where there were mobile tokens called out. Think I should double check with them on this?
There was a problem hiding this comment.
Oh, hmmm... so it's behaving the same way it did before pretty much, but you couldn't really tell previously, because we didn't have header content in our template, am I understanding that correctly?
If that's the case, I still think it's a good idea to reach out to design if the topic wasn't surfaced in design review, but since you already got design review and this isn't a new feature, we can make it a follow-up item rather than a blocker.
There was a problem hiding this comment.
Correct- main doesn't support the extra header content. If I manually input a div into the browser on main, the results are very similar (with some style bugs)
Browser, dialog on production, viewport < 700px:
marissahuysentruyt
force-pushed
the
marissahuysentruyt/css-714-s2-dialog
branch
from
5c3c954
to
179f3f8
Compare
- add hasCheckbox, header args - adds new features/behaviors such as the checkbox in the footer, wrapping for footer/header content, being able to have close button and button group - removes Divider support in dialog - removes fullscreen/fullscreenTakeover layouts until guidances has been determined
lots of mods are renamed to use "standard dialog" language
- 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
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