The Dialog Component

[supernova-at]

Description

This PR introduces a new component into VeniaUI: The Dialog component.

This component is meant to standardize the way Venia shows Dialogs all over the application and make them easier to use for developers. Now when you need a Dialog you can simply

        <Dialog
            onCancel={onCancel}
            onConfirm={onConfirm}
            title={'A Simple Dialog'}
        >
            <p>Hi, I'm the Dialog component.</p>
        </Dialog>

Instead of having to write or copy / paste a bunch of JSX and matching CSS.

The Storybook entries included in this PR illustrate the capabilities of Dialog:

• It masks the page content behind it
• It is configurable via the isModal prop whether clicking the mask closes the Dialog
• It has a "heading" section that displays a title and a close "X" button
• Its body section displays the props.children passed to it, and appends "Cancel" and "Confirm" buttons at the end, sometimes requiring the user to scroll to see them¹
• The "Cancel", "X", and mask buttons invoke the provided onCancel callback function
• The "Confirm" button invokes the provided onConfirm callback function
• The title, cancel, and confirm texts are all customizable
• It renders and wraps its children in an informed Form component
• It supports passing initial values to its internal Form component
• The onConfirm callback is passed to the internal Form element via its onSubmit prop. In other words, clicking the "Confirm" button will submit the form
• The "Cancel" and "X" buttons reset the form back to its initial state
• It supports disabling all buttons (useful for when network requests are in flight)
• Supports being in the right drawer for mobile mode

¹: This is per UX. The "Cancel" and "Confirm" buttons should not be sticky / always appear.

This PR also implements this new Dialog component on the Checkout page Shipping Methods section. Edit shipping methods on the checkout page to see it.

Note that there are other instances that should be converted that have been broken out into their own issues: Shipping Information and Payment Information on Checkout Page, and the Product Filters.

Feedback Requested

Here are some specific areas to focus on for feedback:

The Internal Form

Dialog renders a Form element internally.

For Buttons' Sake

This is primarily so that the Dialog buttons can be part of and control the form. For example, the close "X" and "Cancel" buttons reset the form, and the "Confirm" button submits the form.

The obvious alternative is to have the caller pass in a form of their own as children to the Dialog.
But then the Dialog buttons would be outside of the form and wouldn't have any way to access / control it.

There are some options to explore here, like having the Dialog export its buttons separately for consumers to inject into their forms:

/* MyComponent.js */

import Dialog, { DialogButtons } from 'Dialog';

// render...
<Dialog>
  <Form>
    <MyFormFields />
    <DialogButtons />
  </Form>
</Dialog>

This pattern feels pretty non-idiomatic to me though, so I chose not to introduce it.

But the Mask

Unfortunately the button that is the mask is outside of the form, and therefore cannot / does not reset the form when clicked.

You can reproduce this by changing a form field value and clicking the Mask to close the Dialog.
When you re-open the Dialog, your change persists. This is in direct contrast to how the close "X" and "Cancel" buttons work (they reset the form back to its initial values on click).

Here's the tradeoff I ran into:

/*
 * This PR.
 * Bad because clicking the Mask doesn't reset the form.
*/

<Modal>
  <Mask />
  <aside>
    <Form>{children}</Form>
  </aside>
</Modal>

/*
 * Alternate approach.
 * Bad because semantic HTML?  Fullscreen Mask inside Form feels bad.
 */
<Modal>
  <aside>
    <Form>
      <Mask />
      {children}
    </Form>
  </aside>
</Modal>

Update: We've decided to go with the Mask inside the Form. It isn't bad semantically.

Props to the Form

Currently, Dialog supports setting the initialValues and onSubmit props of the Form via its initialFormValues and onConfirm props respectively.

But Form does support other props. Should Dialog take a form prop (or similar) that it just spreads out onto the Form?

/* Dialog */

const { form: formProps } = props;

<Form {...formProps} onSubmit={onConfirm}>
</Form>

This would allow for more flexibility in working with Dialog's internal Form at the cost of having a slightly more convoluted call site:

const formProps = {
  initialValues: { name: 'some name' }
};

<Dialog formProps={formProps} />

Update: Support added for the pass through of formProps to Dialog's internal Form in 5d8032b.

Dialog Buttons

Should Dialog be configurable to show only one button? To not show any buttons (just the close "X")? These could be useful for more informational-type system messages. There is overlap with Toasts here.

Update: For now the Dialog will always show two buttons. In Modal mode, where the user is intentionally forced to interact with the content, the close "X" button is not rendered and the mask behind the dialog is not clickable. This makes the "Cancel" button an explicit, intentional rejection by the user and the "Confirm" button an explicit, intentional acceptance.

Related Issue

Closes PWA-486.

Acceptance

Verification Stakeholders

@soumya-ashok @jimbo

Specification

Verification Steps

Storybook

To see the Storybook for the Dialog component, run

yarn workspace @magento/venia-ui run storybook

and click on Dialog in the left navigation.

The Checkout Page

The Dialog component is also implemented on the Checkout page, for editing shipping methods.

1. Add an item to your cart
2. Go to the /checkout page
3. Set a shipping address and an initial shipping method
4. Click to "Edit" your Shipping Method
5. Verify that the Dialog appears
6. Verify that "Cancel" and "Update" buttons appear at the end of the body content of the Dialog (you likely have to scroll)
7. Verify that clicking the gray mask closes the Dialog
8. Note your selected shipping method
9. Open the Dialog back up and change your selection
10. Verify that clicking "Update" updates your shipping method (it is updated on the page when the Dialog closes)
11. Open the Dialog again and change your selection
12. Verify that clicking "Cancel" and the "X" do not update your selected shipping method
13. Open the Dialog again
14. Verify that your current shipping method is selected not the one you cancelled out of
15. Switch to mobile view
16. Verify that the Dialog appears in the right drawer and functions correctly

Screenshots / Screen Captures (if appropriate)

This new Dialog component as seen on the Checkout page:

Checklist

• I have updated the documentation accordingly, if necessary.
• I have added tests to cover my changes, if necessary.

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-486.

Generated by 🚫 dangerJS against 9b148f8

[luongvm]

I have a comment: with informed, every time you want to manipulate the form from within the parent of the <Form> element, you need to wrap it with another element / use HOC. This introduces extra element and prop passing pattern which I found hard to follow.

I have looked at https://github.com/wsmd/react-use-form-state#examples and it seems like you can use/manipulate the form without the extra layer/HOC.

With this you don't need to embed a form element to the dialog itself.

Regards,
L.

[jimbo]

@supernova-at Nice work so far. As discussed, i think the it's fine to render a form as a child here, but I do prefer the alternate masking approach, in which the mask is a child of the dialog.

[jimbo]

Consider 100% instead of using viewport units. Apparently mobile browsers have issues with viewport units, especially vh.

[supernova-at]

Fixed in d5841ab.

[jimbo]

This is certainly one approach, but I think this is far too tricky for a one-element grid.

Try removing grid-template-areas, grid-template-columns, and grid-template-rows, then using align-content and justify-content to center the dialog element.

Remember to remove grid-area from the grid item (.container) when you do so.

[supernova-at]

Fixed in 405e489.

[jimbo]

These values feel pretty arbitrary to me, given that we often aim to stick to increments of 1rem. What about 768px by 432px, which is 48rem by 27rem?

[supernova-at]

Fixed in d5841ab.

[jimbo]

Thanks for the fix. When we reviewed this with UX, didn't we propose shrinking the dialog to something like 640x360?

[supernova-at]

Yes - that change is in here: 805a399.

[jimbo]

Likewise, I don't know that grid-template-areas provides value here, since repositioning the header and body of a dialog doesn't really qualify as a use case. Consider dropping this and just allowing grid-template-rows to set up the tracks for grid items to flow in naturally.

[supernova-at]

Fixed in d5841ab.

[jimbo]

This is good. It's probably also a good idea to define the columns here.

/* could even leave off the last `auto` */
grid-template-columns: 1fr auto;

[supernova-at]

Fixed in d5841ab.

[jimbo]

Likewise, increments of rem, so probably at least 1rem here.

[supernova-at]

Fixed in d5841ab.

[jimbo]

I see what your intent is here, but this doesn't feel right. If i were writing this component, I might have called this one shouldDisableButtons, since props are essentially recommendations that the component uses to determine final state.

// disable the mask if a parent says we should
const maskIsDisabled = isModal || shouldDisableButtons

[supernova-at]

Fixed in d5841ab.

[supernova-at]

Fun side note: I changed this and a snapshot that I wasn't expecting to change changed and I was able to fix the bug before anyone else ever saw it - yay snapshots!

[jimbo]

It's good to be able to customize these buttons. Perhaps we should also have another prop for whether to render them at all?

const { includeButtons } = props

const buttons = includeButtons
  ? // create button elements
  : null

[larsroettig]

Maybe it makes sense to have own buttons as a list with your own actions make it very changeable

[jimbo]

The issue with accepting any list of buttons is that this component is responsible for defining and attaching listeners to at least the standard buttons. If button customization is important, we may need a slightly more sophisticated API.

const { buttons } = props

const buttonElements = Array.from(buttons, button => {
  const { key, props } = button

  // `DialogButton` could render a `Button`,
  // attach a listener, and call an optional callback
  return <DialogButton key={key} {...props} />
})

[supernova-at]

I think we decided against an option supporting this.

[jimbo]

Do we really need to do this? I have a feeling these overrides aren't necessary and may be working around either a bug or a mistake.

[supernova-at]

Yeah I don't know why I couldn't get <Button priority="high"> to work. I'll try again.

[supernova-at]

Resolved in 79dc8d1.

[supernova-at]

PR Updated:

• The HTML hierarchy has been simplified and the mask is inside the form, allowing it to act as a "reset" button

Unfortunately I still can't get the buttons in the button container to behave how I want them to when long button texts are set. The easiest place to see this is in the "Customizing the Button Texts" story in Storybook:

I'm pretty confident now that the problem is in the interplay of our Button component and the grid.
You can see that the buttons extend past their grid cells, making it appear as though the grid gap isn't being honored.

If someone cough @jimbo cough wants to mess around with it further that would be awesome but since I've been looking at it for two days straight now I'm inclined to go with the simple approach of stacking the buttons on mobile and putting them side-by-side on desktop, no matter how long their content is.

[devops-pwa-codebuild]

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2365.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.65

[jimbo]

If someone cough @jimbo cough wants to mess around with it further that would be awesome but since I've been looking at it for two days straight now I'm inclined to go with the simple approach of stacking the buttons on mobile and putting them side-by-side on desktop, no matter how long their content is.

I think that's a reasonable solution, and I'd defend it. I'll riff on the layout for a bit and see if I can identify what's causing the buttons to not participate correctly, but we should probably just take your solution and switch from columns to rows on mobile.

[supernova-at]

I believe the build is failing due to the lighthouse performance check.

@dpatil-magento I thought this was more of a warning than an error?

[jimbo]

@supernova-at Good changes, I think we're ready to work with this. Should be easy to apply new styles when they're ready. 👍

[dpatil-magento]

@supernova-at In mobile Shipping methods button are vertically displayed instead of horizontal.