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

Move the verification request from CampaignAssetsForm to AdaptiveForm #2014

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

eason9487
Copy link
Member

@eason9487 eason9487 commented Jul 14, 2023

Changes proposed in this Pull Request:

This is a pre-processing PR for #634 and for 📌 Display form validation errors in #1993.

To have a form state to determine whether to start showing validation errors to users, this PR:

  • Add the relevant tests for the CampaignAssetsForm component first.
  • Add basic tests for the AdaptiveForm component.
  • Move the state and functions associated with the request verification from CampaignAssetsForm to AdaptiveForm.

Screenshots:

Kapture.2023-07-14.at.18.22.16.mp4

Detailed test instructions:

This PR only changes the code used in the campaign assets management.

  1. Start creating a campaign and proceed to step 2.
  2. Import a Final URL.
  3. Click the "Save changes" button with invalid asset form data.
  4. Check if the UI shows validation errors and also scroll page to the position of the first error.
  5. Click the "Or, select a different Final URL" button to see if the UI hides all validation errors.
  6. Repeat this test steps 3-5 again to see if the behavior of validation errors still works well.

Changelog entry

@eason9487 eason9487 requested a review from a team July 14, 2023 10:40
@eason9487 eason9487 self-assigned this Jul 14, 2023
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Jul 14, 2023
@eason9487 eason9487 changed the title Move the request verification request from CampaignAssetsForm to AdaptiveForm Move the verification request from CampaignAssetsForm to AdaptiveForm Jul 14, 2023
Copy link
Contributor

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for the tests and the refactoring. The error messages shown as expected, LGTM.

@eason9487 eason9487 merged commit 6fe37e8 into develop Jul 18, 2023
6 checks passed
@eason9487 eason9487 deleted the dev/634-elevate-form-validation-request branch July 18, 2023 09:48
@martynmjones martynmjones mentioned this pull request Aug 1, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants