Skip to content
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

Correct dialog form values after submission #2960

Merged
merged 10 commits into from
Jan 27, 2021
Merged

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Jan 21, 2021

Description

This ticket originally called for the submit / confirm handler to clear out form values like the reset / cancel button does.

Unfortunately, that didn't end up being viable. Take the following code addition to the Dialog component itself:

    const onSubmit = useCallback(
        async args => {
            if (onConfirm) await onConfirm(args);
            formApiRef.current.reset();
        },
        [onConfirm]
    );

We actually only want to reset the form when onConfirm is successful. When this function results in an error, we want to keep the form fields in their current state so that the user can more readily fix the problem.

Unfortunately, there's no established pattern or enforcement of a return value from the onConfirm callback. In fact, most of the ones in the Venia storefront return undefined, explicitly or implicitly.

It feels very heavy handed and error prone for the Dialog component to suggest or require such a return value from its callers.

As such, I think the best approach for now is a targeted one where callers set shouldUnmountOnHide to true on their Dialogs when they want the reset functionality. The form will be re-mounted when the Dialog reappears on screen, and the (new, updated)initialValues will be used.

To facilitate this, this PR changes the default value for the Dialog shouldUnmountOnHide prop to true.
Any instance of Dialog that was explicitly setting shouldUnmountOnHide to true has been updated to omit that prop, and any instance of Dialog that was missing shouldUnmountOnHide / relying on the default value has been updated to explicitly set shouldUnmountOnHide to false.

Related Issue

Closes PWA-961.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Sign In
  2. Navigate to the /account-information page (My Account > Account Information)
  3. Click the "Edit" button
  4. Change the "Last Name" field and enter the correct password
  5. Keep a close eye on the "Last Name" field and click "Submit"
  6. Verify that there was no flash of old content in the "Last Name" field
  7. Re-open the "Edit" Dialog by clicking on the "Edit" button
  8. Verify that the "Last Name" field is correctly updated to the new value and that the password field has been cleared out

Screenshots / Screen Captures (if appropriate)

Screen Shot 2021-01-21 at 12 39 51 PM

Checklist

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

Risks

Do we need to make this change anywhere else?

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jan 21, 2021

Messages
📖

Associated JIRA tickets: PWA-961.

📖 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).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 53e6bf0

@schensley
Copy link

Reviewed via EDIT Account Information and EDIT Shipping Address in Checkout. Behaves as expected. UX Approved.

@@ -180,5 +180,5 @@ Dialog.defaultProps = {
confirmText: 'Confirm',
confirmTranslationId: 'global.confirmButton',
isModal: false,
shouldUnmountOnHide: false
shouldUnmountOnHide: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution unmounts/destroys the component meaning we lose animations.

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Meant to request changes :D

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several files that did not use this prop (meaning they used the default of false). Should we add shouldUnmountOnHide={false} to those <Dialog> implementations?

I also have concerns about losing the transitions. I'm not sure it was intentional otherwise I'd expect you to have removed related CSS.

@supernova-at
Copy link
Contributor Author

There are several files that did not use this prop (meaning they used the default of false). Should we add shouldUnmountOnHide={false} to those implementations?

Yes, great idea. I can definitely do that.

This solution unmounts/destroys the component meaning we lose animations.

Very valid concern but we have a separate ticket (PWA-1194) to restore the close animations when shouldUnmountOnHide is true so I didn't do any of that in this scope.

sirugh
sirugh previously approved these changes Jan 26, 2021
@dpatil-magento
Copy link
Contributor

@supernova-at Tried verification steps and Password field still not cleared -

Image from Gyazo

@supernova-at
Copy link
Contributor Author

@supernova-at Tried verification steps and Password field still not cleared -

Lol oops, accidentally flipped that one twice.

Good to go in 053b977.

@supernova-at supernova-at added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jan 27, 2021
@dpatil-magento dpatil-magento self-requested a review January 27, 2021 22:00
@dpatil-magento
Copy link
Contributor

QA Approved

@dpatil-magento dpatil-magento merged commit 330d6ff into develop Jan 27, 2021
@dpatil-magento dpatil-magento deleted the supernova/961 branch January 27, 2021 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:venia-ui Progress: done version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants