306 add a11y eslint package #330
Conversation
For the collapsible sections in the control panel
Changes made in 269 may help with a11y conformity
gsproston-scottlogic
requested review from
chriswilty,
pmarsh-scottlogic,
heatherlogan-scottlogic and
dhinrichs-scottlogic
|
Looks like most of the work isn't to do with adding the a11y package, and I'm struggling to review the overlay stuff because I don't have the context, or a linked ticket etc. A screenshot would be handy! |
Yeah as I mentioned I pulled in some work from #269 as it uses interactive html elements (which don't set off a11y linting errors) so I'll wait until #318 is approved before merging this. There is a linked ticket under the 'development' section on the right, it's #306 . And yes, the overlay now looks like this:
It now has an 'x' to close it. |
|
@gsproston-scottlogic Can you please change the base/target branch to the grouped defenses branch, so we can see what is specific to this PR? Thanks. You'll need to merge that branch first anyway, and once that is merged, github will automatically change the base branch to main. |
|
What eslint rules are breaking by using escape-to-close? That's a pretty universal thing (as is clicking outside to close), whereas an X button to close is not so user-friendly. |
gsproston-scottlogic
changed the base branch from
dev
to
269-group-defences
Yes can do! Changed now 👍 |
It's to do with how the current overlay works. There's an outer "handbook-overlay-screen" div which covers the entire screen, and an child "handbook-overlay" div in that which is the actual overlay bit. I had it so both of these divs had click handlers. The parent to close on click, and the child to stop click propagation.
The a11y package didn't like that there were click handlers on divs, and same with key press handlers. In the end I copied our current solution to the 'View Documents' overlay, which simply uses an 'x'. I wasn't quite sure how to make this fully accessible 🤔 We do have another ticket #319 for the overlay, so perhaps we could cover it there? |
@gsproston-scottlogic Yeah that makes sense, as they cannot be focused. The click and keydown handlers would need to be bound to the window, rather than on the div element of the overlay. That can be done in a useEffect (which could also unbind the event handlers on closing the overlay). Yep, feel free to tackle this in #319 instead. |
| <div id="handbook-overlay-screen"> | ||
| <div id="handbook-overlay"> | ||
| <button | ||
| id="close-button" |
There was a problem hiding this comment.
Avoid using html id unless you absolutely need to. Ids must be unique in a document.
I see you are using ids to apply styling, when using a classname is the preferred way. Easy to fix: change id to className in your components, and # to . in the css file.
There was a problem hiding this comment.
Just changed ids to classNames here, and in MainHeader.tsx too. There's a bunch of other instances in the code where we can switch to className, maybe all of them. I could raise a refactor ticket for it?
| border: none; | ||
| border-width: 1px; | ||
| border-style: solid; | ||
| border-color: transparent; |
There was a problem hiding this comment.
FYI You can collapse these: border: 1px solid transparent;
| <div id="handbook-overlay-screen"> | ||
| <div id="handbook-overlay"> | ||
| <button | ||
| id="close-button" |
* aria-labels for handbook and info buttons * Better label for defence toggle checkbox
* Grouped defences * Better html grouping * Removed strategy box * Visually clearer grouping * Rounded corners on control section * Added the package * Fixed issues in ModelConfigurationSlider * Fixed issues in MainHeader * Fixed issues in DocumentViewBox * Removed autofocus for accessibility considerations * Fixed issues in ApiKeyBox * Using some rather than find * Using details html elements For the collapsible sections in the control panel * Better html DRY principles * Fixed issues in DefenceMechanism * Accessible overlay * No longer disabling eslint rule for file * Using border rather than outline for button hover * Allowing autofocus for chat input * Better overlay close buttons * Aria labels for overlay close buttons * Using class names for styling in handbook overlay * Handbook icon class name * 322 wrap handbook image in a button and give aria label (#341) * aria-labels for handbook and info buttons * Better label for defence toggle checkbox
Would be interested in people's thoughts on how the overlay works. Can't make it so it can be clicked off, or for the user to press 'escape' to close without breaking some eslint rules. Could maybe push that work to #319
Also pulled in work from #269 as that uses some nice interactive elements.