-
Notifications
You must be signed in to change notification settings - Fork 0
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
Edit form contact details #462
Conversation
davidjamesstone
commented
Sep 10, 2024
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ace work @davidjamesstone
Some minor comments, see what you think
const { slug } = params | ||
const { token } = auth.credentials | ||
|
||
const validation = yar.flash(sessionNames.validationFailure).at(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried how much we're sharing sessionNames.validationFailure
You could potentially submit a form in one form and see the errors in another (separate tab)
Should we keep the same name, but use a nested object keyed by route name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good point. Future improvement?
document.querySelector('#address') | ||
) | ||
|
||
expect(address?.value).toBe('[email protected]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not a fan of coercing everything to HTMLInputElement
, have you seen this?
expect(address?.value).toBe('[email protected]') | |
expect(address).toHaveValue('[email protected]') |
Although, can we start switching to ByRole
queries so we're testing labels + hints at the same time?
I've been following the $XXX
syntax for queried elements that GOV.UK Frontend uses too:
expect(address?.value).toBe('[email protected]') | |
const $address = screen.getByRole('textbox', { | |
name: 'Email address', | |
description: 'XXX' | |
}) | |
expect($address).toHaveValue('[email protected]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved with 50f25b4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @davidjamesstone
What about the ByRole
queries?
We're currently using field IDs but "role" queries also test if labels/hints etc are wired up too
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
I'll add a fix to my code coverage PR, but there was a compiler error in this one:
|
Fixed in commit range |