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

bugfix/1912-Support-Page-Forms-Validation #1961

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

velnachev
Copy link
Contributor

@velnachev velnachev commented Oct 17, 2024

Implemented dynamic form validation for each step of the support form

Closes #1912

Motivation and context

On blur validation has been disabled due to an issue caused by the initial focus on the first input field of a form step

Logic is that each step will validated on change only when it was submitted with an error from the validation schema for that step. If a user goes back to a previous step and tries to submit it again with an error that logic will again be triggered

Includes a small fix for the deprecated Hidden MUI component. Fixed as per suggestion in MUI documentation

Testing

Steps to test

  1. Navigate to https://podkrepi.bg/en/support
  2. Try to submit each step without selecting/filling anything
  3. Observe validation behavior for each step
  4. Go back to previous steps and try to submit without selecting/filling anything and observe validation behavior

Affected urls

Environment

New environment variables:

  • NEW_ENV_VAR: env var details

New or updated dependencies:

Dependency name Previous version Updated version Details
dependency/name v1.0.0 v2.0.0

Copy link

github-actions bot commented Oct 17, 2024

✅ Tests will run for this PR. Once they succeed it can be merged.

@gparlakov gparlakov added the run tests Allows running the tests workflows for forked repos label Oct 18, 2024
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Oct 18, 2024
@gparlakov
Copy link
Contributor

@velnachev hi. This "ValidationObserver" pattern does not seem to have been used before in the project. Help me understand where it comes from and what makes it the right fit for the project that uses formik

@velnachev
Copy link
Contributor Author

velnachev commented Oct 19, 2024

@velnachev hi. This "ValidationObserver" pattern does not seem to have been used before in the project. Help me understand where it comes from and what makes it the right fit for the project that uses formik

Hi! The pattern has not been used in the project since I've developed it to solve the particular issue and improve the UX of the form flow by displaying error states and triggering the onChange validation of the form only once an erroneous submission attempt has been made for the particular step, instead of it being immediately triggered once a field is touched.

Formik is certainly being used, and the role of the ValidationObserver component is to simply gain access to the formik context, so the logic of tracking submission attempts for each separate step in a state variable can be achieved, which is then used in toggling the onChange validation of formik on and off.

If this is not a desired behavior - we can just do validateOnBlur={false} on the form and the described issue would be solved.

Edit: Actually going back to the ticket now, and we are happy with the described behavior, I think there is an easier way of achieving it, so please let me know so I can look into reworking it when I get a chance

Copy link
Contributor

@gparlakov gparlakov left a comment

Choose a reason for hiding this comment

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

Thank you for your response,

I believe I understand the intent now after reading the comment a few times and also trying to fix the issue using alternative means. So now that I understand the code I have a few suggestions that only aim at making the logic more readable and understandable.

See them in the individual files' comment

</Hidden>
innerRef={formRef}
validateOnBlur={false}
validateOnChange={currentValidatedField === activeStep}>
Copy link
Contributor

Choose a reason for hiding this comment

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

currentValidateField sounds like it pertains to a field, but in here it is used to toggle on/off of individual field validation on a step.

How about validateInidividualFieldsOnStep ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to make the pattern more explicit and readable by making the value that comes out of the ValidationObserver a map such that individual field validation is turned on/off explicitly

<GeneralForm<...>
      ...
      validateOnBlur={validateIndividualFields[activeStep]}
      validateOnChange={validateIndividualFields[activeStep]}>
      

[Steps.PERSON]: ['person'],
}

const ValidationObserver = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some comment stating the need to turn off individual field validation first time the step is submitted but then turning it back on so the user gets immediate feedback when filling the correct/expected value

Copy link
Contributor

Choose a reason for hiding this comment

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

And finally - this new pattern might benefit greatly from a few unit tests. Benefit in terms of readability.

@velnachev
Copy link
Contributor Author

Thank you for the review! I'll include those when I get a chance and if I cannot fix it in the simpler alternative way I think could be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When show error message on support page
2 participants