-
Notifications
You must be signed in to change notification settings - Fork 21
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
Plat UI 3352 maintain links to error and hints #434
Plat UI 3352 maintain links to error and hints #434
Conversation
src/components/accessible-autocomplete/accessible-autocomplete.browser.test.js
Outdated
Show resolved
Hide resolved
id: location-picker | ||
select: | | ||
<select id="location-picker" data-show-all-values="false" data-auto-select="false" data-default-value="" data-module="hmrc-accessible-autocomplete"> | ||
<select class="govuk-select govuk-select--error" aria-describedby="location-picker-error location-picker-hint" id="location-picker" data-show-all-values="false" data-auto-select="false" data-default-value="" data-module="hmrc-accessible-autocomplete"> |
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.
had to add these, because our markup wasn't actually what a select in error state would look like so VRT will have been wrong for this
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 wasn't the styles broken, but the markup of the example being incorrect
src/components/accessible-autocomplete/accessible-autocomplete.browser.test.js
Show resolved
Hide resolved
new MutationObserver(() => { | ||
const currentAriaDescribedBy = autocompleteElement.getAttribute('aria-describedby') || ''; | ||
if (!currentAriaDescribedBy?.includes(selectElementAriaDescribedBy)) { | ||
autocompleteElement.setAttribute('aria-describedby', `${selectElementAriaDescribedBy} ${currentAriaDescribedBy}`); | ||
} | ||
}).observe(autocompleteElement, { | ||
attributes: true, | ||
attributeFilter: ['aria-describedby'], | ||
}); |
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.
it's important to have the check to see if it's already ok - so that you don't get an infinite loop of updates triggering more updates
343efdc
to
086d941
Compare
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.
the example markup for an autocomplete field with an error was incorrect so while we fixed this issue in a previous release, you couldn't see it reflected in the tests - I've updated the example that the tests use now, so this is just a test change
I've updated the test coverage, checked that a js error in the browser would fail tests, and that the fixes work in the latest browsers in windows/macs/ios/android |
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.
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.
LGTM
No description provided.