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

fix: Confirm Dialog allows badly formatter code to be submitted #3072

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

wmoussa-gc
Copy link
Contributor

@wmoussa-gc wmoussa-gc commented Jan 3, 2024

Summary | Résumé

Error code missing in confirmation modal

The user copied and pasted submission IDs into the confirmation modal and got a message that it was sending. They didn't get an error that it was the wrong format.

https://app.zenhub.com/workspaces/gcforms-60cb8929764d71000e481cab/issues/gh/cds-snc/platform-forms-client/3027

For any UI changes please include screenshots at different breakpoints. It would also be helpful to include a
before and after comparison for each change i.e.

Before After
image image

Test instructions | Instructions pour tester la modification

Sequential steps (1., 2., 3., ...) that describe how to test this change. You should include
anything outside the README that will help the developer access your changes and be able to test
them such as any environmental setup steps and/or any time-based elements that this requires.
This will help a developer test things out without too much detective work.

Unresolved questions / Out of scope | Questions non résolues ou hors sujet

Are there any related issues or tangent features you consider out of scope for
this issue that could be addressed in the future? If so please create issues and provide
links in this section

Pull Request Checklist

Please complete the following items in the checklist before you request a review:

  • Have you completely tested the functionality of change introduced in this PR? Is the PR solving the problem it's meant to solve within the scope of the related issue?
  • The PR does not introduce any new issues such as failed tests, console warnings or new bugs.
  • If this PR adds a package have you ensured its licensed correctly and does not add additional security issues?
  • Is the code clean, readable and maintainable? Is it easy to understand and comprehend.
  • Does your code have adequate comprehensible comments? Do new functions have docstrings?
  • Have you modified the change log and updated any relevant documentation?
  • Is there adequate test coverage? Both unit tests and end-to-end tests where applicable?
  • If your PR is touching any UI is it accessible? Have you tested it with a screen reader? Have you tested it with automated testing tools such as axe?

@wmoussa-gc wmoussa-gc marked this pull request as draft January 3, 2024 00:24
Copy link
Contributor

github-actions bot commented Jan 3, 2024

@wmoussa-gc wmoussa-gc marked this pull request as ready for review January 3, 2024 00:29
{t(
"downloadResponsesModals.confirmReceiptDialog.errors.invalidEntry.description"
)}
<Trans
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to use the "t" function here

const { t } = useTranslation("name-of-file");

i.e.

t("downloadResponsesModals.confirmReceiptDialog.errors.minEntries.title")

Copy link
Contributor Author

@wmoussa-gc wmoussa-gc Jan 3, 2024

Choose a reason for hiding this comment

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

I know, but I took it up a notch, and now you've got a recipe that lets you style "some words" in your translation string with some basic HTML elements (italic in this case). As far as I've read and tested, the t function doesn't support it. Trans component does it but albeit not super intuitively https://react.i18next.com/latest/trans-component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please let me know if you think it is a bad idea

Copy link
Member

@timarney timarney Jan 3, 2024

Choose a reason for hiding this comment

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

I'm not opposed to it ... to date we've broken strings apart which isn't a nice experience.

For sure it's not intuitive and the code without reading the docs looks like a mistake i.e. the empty <i></i>

Maybe leave a comment above the i tags to help any confusion on that end ... just something helpful for the next dev to edit that code.

Can we try using named tags? Might be helpful for folks doing the translations (thoughts?)

Make sure you also adapt your translation resources to include the named tags () instead of the indexed tags (<0>)!

Copy link
Member

Choose a reason for hiding this comment

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

import { Trans } from "next-i18next";

Re: the import + using Trans

@bryan-robitaille any concerns thinking about the Next JS update + translations 👆

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of any conflicts at the moment. In the Next JS update the i18n is initialized separately on the server and the client and the next-i18next package is removed. We'll be using pure react-i18next and it 'should' work as long as the initialized t function is passed in.
I completely agree about using named tags as the indexes are not very straight forward and can quickly change when content is updated.

@timarney
Copy link
Member

timarney commented Jan 4, 2024

Looks like there's an issue re-enabling the submit button

  • Enter a bad code
  • Submit button is disabled
  • Remove the bad code without closing the modal
  • Add a valid code
  • Submit button remains disabled but should be valid at this point
Screenshot 2024-01-04 at 3 42 55 PM

Copy link
Member

@timarney timarney left a comment

Choose a reason for hiding this comment

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

Re-tested

Works well.

Nice job @wmoussa-gc .

@wmoussa-gc wmoussa-gc merged commit ca65398 into develop Jan 5, 2024
11 checks passed
@wmoussa-gc wmoussa-gc deleted the bug-fix-confirmation branch January 5, 2024 21:40
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.

3 participants