spectrum-css icon indicating copy to clipboard operation
spectrum-css copied to clipboard
verified publisher icon adobe

feat(dialog): s2 dialog migration standard/takeover

Open marissahuysentruyt opened this issue 1 year ago • 7 comments

Description

This migrates the Dialog to S2. 🎉 The more specific names are Standard Dialog and Takeover Dialog (which may help distinguish it from the separate Alert Dialog component). 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 with info on the takeover dialog variants and the divider story was removed since dividers are no longer supported in dialog components in S2.

This PR pulls in certain features from main in preparation for when spectrum-two merges. That includes some import statements that are commented out and the dialog.test.js file. 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-takeover-dialog-spacing-grid-padding --mod-takeover-dialog-spacing-header-gap

Renamed 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-group --mod-dialog-fullscreen-header-text-size >--mod-takeover-dialog-title-font-size

Removed 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-height

Designs

S2 Standard Dialog Tokens specs S2 / Desktop Standard Dialog S2 Takeover Dialog Token specs S2 / Desktop Takeover dialog

There will be subsequent work for standard dialog, specifically for fullscreen layouts. Defining "flexible dialog and modal is something currently being scoped for our Q3 roadmap," so CSS-829 was made as a placeholder for that work.

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

  • [x] Standard dialog design review and approval @marissahuysentruyt CSS-803
  • [ ] Takeover dialog design review and approval
  • [ ] Pull down the branch
  • [x] Visit the dialog storybook
  • [x] The standard dialog matches the S2 standard dialog specs
  • [ ] Visit the fullscreen dialog and fullscreenTakeover dialog
  • [ ] The fullscreen & fullscreen takeover dialog stories matches the s2 takeover dialog specs
  • [ ] The dialog no longer supports the Divider component, so any JS or CSS referencing the dialog's divider has been removed from the stories and index.css files
  • [x] Verify the .spectrum-Dialog and .spectrum-Modal utilize the updated border-radius value (corner-radius-extra-large-default 16px)
  • [x] Verify the button group or close button is keyboard accessible and the focus rings do not get cut off in any variant
  • [ ] In your editor, verify that the class modifiers --small, --medium and --large have been removed from the JS and CSS. Any sizing modifiers should now reflect T-shirt sizing (--sizeS, size--L), without a --sizeM as that is the default dialog size.
  • [ ] Create various combinations of the dialog to verify combos are considered in the CSS:
    • [x] mobile vs desktop
    • [x] light vs dark mode
    • [x] "small", "medium", "large" dialogs to test the max widths
    • [x] dismissible vs. non-dismissible
    • [x] hero vs non-hero
    • [x] changing the hero image
    • [ ] fullscreen and fullscreenTakeover layouts (takeover dialogs do not support a footer or hero images, and are not dismissible)
    • [x] change the optional header content to verify that element wraps
    • [x] change the optional footer content to verify that element wraps
  • [x] You may run into the error "Rendered fewer hooks than expected." when using certain controls. Refreshing the window will remove the error.
  • [x] Verify new tokens are used:
    • [x] standard-dialog-maximum-width-small
    • [x] standard-dialog-maximum-width-medium
    • [x] standard-dialog-maximum-width-large
    • [x] standard-dialog-minimum-width
    • [x] standard-dialog-title-font-size
    • [x] standard-dialog-body-font-size
    • [ ] takeover-dialog-width
    • [ ] takeover-dialog-height
    • [ ] window-to-edge
  • [x] Verify updated tokens are used:
    • [x] corner-radius-extra-large-default
    • [x] spacing-500
    • [ ] spacing-400
    • [x] spacing-300
    • [ ] background-layer-2-color (in the designs, this is accidentally misnamed as background-color-layer2
    • [x] line-height-200
  • [x] Chromatic coverage (using dialog.test.js file) 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.
  • [ ] Tray, which uses a dialog component, doesn't looks broken. It should no longer have a divider.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • [x] The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • [ ] VRTs have been run and looked at.
  • [ ] Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • [x] I have read the contribution guidelines.
  • [x] I have updated relevant storybook stories and templates.
  • [x] I have tested these changes in Windows High Contrast mode.
  • [x] If my change impacts other components, I have tested to make sure they don't break.
  • [x] If my change impacts documentation, I have updated the documentation accordingly.
  • [ ] ✨ This pull request is ready to merge. ✨

marissahuysentruyt avatar Jun 24 '24 15:06 marissahuysentruyt

🦋 Changeset detected

Latest commit: 6505da4293d55694d789ba89eb084858b85cec07

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
@spectrum-css/buttongroup Patch
@spectrum-css/tokens Patch
@spectrum-css/dialog Major
@spectrum-css/modal Patch
@spectrum-css/underlay Patch
@spectrum-css/preview Patch
@spectrum-css/popover Major
@spectrum-css/tray Major
@spectrum-css/alertdialog Patch
@spectrum-css/actionbar Major
@spectrum-css/actionmenu Major
@spectrum-css/coachmark Major
@spectrum-css/combobox Major
@spectrum-css/contextualhelp Major
@spectrum-css/datepicker Major
@spectrum-css/picker Major
@spectrum-css/pickerbutton Major
@spectrum-css/menu Major
@spectrum-css/tabs Major

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

changeset-bot[bot] avatar Jun 24 '24 15:06 changeset-bot[bot]

File metrics

Summary

Total size: 2.74 MB* Total change (Δ): ⬆ 1.20 KB (0.04%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
buttongroup 1.49 KB ⬆ 0.05 KB
dialog 15.09 KB ⬆ 0.40 KB
modal 5.49 KB ⬆ 0.02 KB
underlay 3.18 KB ⬇ 0.10 KB
tokens 472.69 KB ⬆ 0.33 KB

Details

buttongroup

File Head Base Δ
index-base.css 1.49 KB 1.44 KB ⬆ 0.05 KB (3.53%)
index.css 1.49 KB 1.44 KB ⬆ 0.05 KB (3.53%)

dialog

File Head Base Δ
index-base.css 15.09 KB 14.71 KB ⬆ 0.40 KB (2.62%)
index.css 15.09 KB 14.71 KB ⬆ 0.40 KB (2.62%)

modal

File Head Base Δ
index-base.css 5.49 KB 5.48 KB ⬆ 0.02 KB (0.29%)
index.css 5.49 KB 5.48 KB ⬆ 0.02 KB (0.29%)

underlay

File Head Base Δ
index-base.css 3.18 KB 3.28 KB ⬇ 0.10 KB (-2.89%)
index.css 3.18 KB 3.28 KB ⬇ 0.10 KB (-2.89%)

tokens

File Head Base Δ
css/dark-vars.css 46.29 KB 46.29 KB -
css/global-vars.css 68.71 KB 68.71 KB -
css/index.css 234.33 KB 234.17 KB ⬆ 0.16 KB (0.07%)
css/large-vars.css 39.58 KB 39.50 KB ⬆ 0.08 KB (0.21%)
css/light-vars.css 46.25 KB 46.25 KB -
css/medium-vars.css 39.70 KB 39.62 KB ⬆ 0.08 KB (0.20%)
index.css 238.36 KB 238.19 KB ⬆ 0.17 KB (0.07%)
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions[bot] avatar Jun 24 '24 15:06 github-actions[bot]

🚀 Deployed on https://pr-2860--spectrum-css.netlify.app

github-actions[bot] avatar Jun 24 '24 15:06 github-actions[bot]

@rise-erpelding thanks for all of this feedback!

The footer content disappears in fullscreen, is that expected? is the close button allowed to be used for fullscreen mode The hero image control doesn't work in Storybook if I'm on fullscreen mode.

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: https://github.com/adobe/spectrum-css/pull/2860/commits/0bfa1a8bfff5b1cb48d58a0ded45ff8b808535bf) for fullscreen layouts)

I cannot remove fullscreen/fullscreen takeover as an option once I select it

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: https://github.com/adobe/spectrum-css/pull/2860/commits/0bfa1a8bfff5b1cb48d58a0ded45ff8b808535bf

The hero image control doesn't work in Storybook if I'm on fullscreen mode.

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: https://github.com/adobe/spectrum-css/pull/2860/commits/94220e66c53b2312d030250e00f0ed28cbd920e4

I captured it with two dialogs but I just looked again and didn't see the issue.

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 reworked the header and footer grid placements: https://github.com/adobe/spectrum-css/pull/2860/commits/d8cd82f296bb5e6384e887ea4bb20c7f76e0956a I also had to rework some of the template to hopefully 🤞 handle some of the edge cases that I was missing: https://github.com/adobe/spectrum-css/pull/2860/commits/eff80b9fb4e9970d5559c661301d6dab6247ae19

I moved the Chromatic testing templates to template.js too, to match some other requests I've seen on other pull requests.

marissahuysentruyt avatar Jul 05 '24 20:07 marissahuysentruyt

With this finding, was the design team planning to update token values? Or update what is listed on the Figma specs redlines?

@jawinn I have not asked design, that's a good idea. Would you just post this question in the spectrum_design_css channel? Or since I've been in contact with Miruna, should I just ask her?

marissahuysentruyt avatar Jul 15 '24 18:07 marissahuysentruyt

I know this card was just moved to blocked, but I was following up on this since I saw that you made some changes and I wanted to leave a few thoughts here while I have them:

  • Wrapping is no longer an issue, that's awesome! I think Josh already called out the thing about the additional header text ("* Required") being on two lines now. I also noted that the buttons are left-aligned and I wasn't sure if that was an intentional design decision or not (since I'd captured them in a screenshot with right alignment before, I think), but might be a good thing to follow up on.
  • Fullscreen mode looks a lot better and a lot of the previous callouts aren't problems anymore! I know you have some outstanding questions on that, so we can wait to see if those get addressed later. Want to leave a note in case it's confirmed that we should not have footer content for fullscreen mode that this would need to be removed from the controls when fullscreen mode is on. There's a lot going on here but it's looking great!

rise-erpelding avatar Jul 16 '24 15:07 rise-erpelding

TODO: take in the new takeover dialog tokens https://github.com/adobe/spectrum-tokens/releases/tag/%40adobe%2Fspectrum-tokens%4013.0.0-beta.50

marissahuysentruyt avatar Oct 22 '24 19:10 marissahuysentruyt

A few additional cards came out of this work. We'll need to follow up with design a some questions when design reviews happen once again.

  • CSS-1057 to address the modal background color
  • CSS-1044 to get clarity on mobile behavior and layout

marissahuysentruyt avatar Dec 10 '24 13:12 marissahuysentruyt