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

OOC-3753 Updates #203

Merged
merged 1 commit into from
Jul 1, 2024
Merged

OOC-3753 Updates #203

merged 1 commit into from
Jul 1, 2024

Conversation

kathryn-dale
Copy link

  • Captialize optionaltext
  • Add missing validation option
  • Update broken error message

- Captialize optionaltext
- Add missing validation option
- Update error messages to keep consistent with previous
@@ -19,8 +19,6 @@ const messageTemplate = {
maxWords: "{{#label}} must be {{#limit}} words or fewer",
dateRequired: "{{#label}} must be a real date",
dateFormat: "{{#label}} must be a real date",
dateMin: "{{#label}} must be the same as or after {{#limit}}",
dateMax: "{{#label}} must be the same as or before {{#limit}}",
Copy link
Author

Choose a reason for hiding this comment

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

Removed these because they are duplicated

@masuk-kazi98
Copy link

Quick q, why do we need to set the any.required messages in the json? They seem to be okay in prod as is

@kathryn-dale
Copy link
Author

Quick q, why do we need to set the any.required messages in the json? They seem to be okay in prod as is

The default has changed in the merge upstream. All required messages used to be the same by default but the default has changed (the any.required now sets to a "select X" error message rather than "x is required" for radio and checkboxes). To maintain existing behaviour but also bring in the new defaults I've updated the json file.

I spoke with Ge from design and the steer they've always had in the past is to use the XGov default messaging, but the messaging doesn't make sense with the new defaults and question names we have (e.g. "Enter Please Enter your local HPT" would be the new error for the HPT question. Ge has said he's going to look at the error messages now he can see the new defaults and create a ticket to update in the future, but for now wants to maintain the existing behaviour, which means we can't use the "any.required" default that's been brought in by merging in upstream. Any further questions I'm happy to hop on a call and show the original diff :)

@masuk-kazi98
Copy link

Quick q, why do we need to set the any.required messages in the json? They seem to be okay in prod as is

The default has changed in the merge upstream. All required messages used to be the same by default but the default has changed (the any.required now sets to a "select X" error message rather than "x is required" for radio and checkboxes). To maintain existing behaviour but also bring in the new defaults I've updated the json file.

I spoke with Ge from design and the steer they've always had in the past is to use the XGov default messaging, but the messaging doesn't make sense with the new defaults and question names we have (e.g. "Enter Please Enter your local HPT" would be the new error for the HPT question. Ge has said he's going to look at the error messages now he can see the new defaults and create a ticket to update in the future, but for now wants to maintain the existing behaviour, which means we can't use the "any.required" default that's been brought in by merging in upstream. Any further questions I'm happy to hop on a call and show the original diff :)

In that case it seems like an issue with the validationOptions file. if you change it to:

required: "{{#label}} is required",

that should fix it no? Currently it's "Enter {question name}"

@kathryn-dale
Copy link
Author

Quick q, why do we need to set the any.required messages in the json? They seem to be okay in prod as is

The default has changed in the merge upstream. All required messages used to be the same by default but the default has changed (the any.required now sets to a "select X" error message rather than "x is required" for radio and checkboxes). To maintain existing behaviour but also bring in the new defaults I've updated the json file.
I spoke with Ge from design and the steer they've always had in the past is to use the XGov default messaging, but the messaging doesn't make sense with the new defaults and question names we have (e.g. "Enter Please Enter your local HPT" would be the new error for the HPT question. Ge has said he's going to look at the error messages now he can see the new defaults and create a ticket to update in the future, but for now wants to maintain the existing behaviour, which means we can't use the "any.required" default that's been brought in by merging in upstream. Any further questions I'm happy to hop on a call and show the original diff :)

In that case it seems like an issue with the validationOptions file. if you change it to:

required: "{{#label}} is required",

that should fix it no? Currently it's "Enter {question name}"

It would fix it, but the design team in future want to use the XGov defaults, and I'm reluctant to change the default error messaging provided by XGov forms when the steer for the design team is to use the default messaging where they can. Our messages are custom, so I think it should be updated in the form. That way our code is a close to upstream as possible, and next time we merge with upstream we won't run into the same issues, as our custom changes will be front loaded and not touch the backend

@kathryn-dale kathryn-dale merged commit ffbf804 into develop Jul 1, 2024
@kathryn-dale kathryn-dale deleted the merge-upstream branch July 1, 2024 10:43
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.

2 participants