319 overlay accessibility reworked #391
Conversation
chriswilty
requested review from
gsproston-scottlogic,
heatherlogan-scottlogic and
dhinrichs-scottlogic
chriswilty
force-pushed
the
319-overlay-accessibility-reworked
branch
from
3003a3f to
5efb0cb
Compare
| ) : ( | ||
| "" | ||
| ); | ||
| ) : null; |
There was a problem hiding this comment.
Note: if you don't want to render anything, return null or undefined, not empty string.
| button { | ||
| height: 100%; | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
This was being applied to all buttons because the css selector is non-specific. Nest styles in top-level classes wherever possible, e.g. this could've been
.export-chat-link > button {
| @media (min-width: 768px) { | ||
| body { | ||
| font-size: 1.125rem; | ||
| } |
There was a problem hiding this comment.
Again, this was being applied universally to the body, and affecting all our content. Always exercise caution when applying styles to an element without any preceding selectors.
|
|
||
| return () => { | ||
| window.removeEventListener("keydown", handleEscape); | ||
| window.removeEventListener("click", handleOverlayClick); |
There was a problem hiding this comment.
@gsproston-scottlogic Adding event listeners to the window means we don't get linter warnings about clicks bound to a non-interactive element. We just need to remember to remove them on unmount.
| (event: MouseEvent) => { | ||
| contentRef.current && | ||
| !event.composedPath().includes(contentRef.current) && | ||
| closeOverlay(); |
There was a problem hiding this comment.
Nice modern way to check if an event was triggered by an element or any of its children. If the click was within the content element then we ignore it, else we close the overlay.
chriswilty
force-pushed
the
319-overlay-accessibility-reworked
branch
from
5efb0cb to
8ee06ec
Compare
chriswilty
force-pushed
the
319-overlay-accessibility-reworked
branch
from
8ee06ec to
2d1e524
Compare
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| window.addEventListener("keydown", handleEscape); |
There was a problem hiding this comment.
As I understand it, we shouldn't need to listen for escape presses to close the modal, the dialog element should handle that for us!
There was a problem hiding this comment.
Correct, but we need our state to update, and when the dialog closes itself without us intercepting the keypress, we don't get that state update so the code still thinks the dialog is open. And our event listeners are still bound.
This is a good example of a controlled react component. We tell the dialog what state to be in, rather than listening for changes and reacting after the fact.
There was a problem hiding this comment.
ah right makes sense!
|
@heatherlogan-scottlogic I've removed the Tools tab. Note that if we also remove the Mission Info, we'll only have Attacks remaining in the handbook... Just saying! |
chriswilty
force-pushed
the
319-overlay-accessibility-reworked
branch
from
ef3d80d to
c9e6f37
Compare
I remember there being talk about having a handbook section on the architecture of the backend (what LLMs are being used and how), and also a glossary page for certain terms. So we can fill up the handbook with more stuff if we like. |
* Make overlay and handbook accessible * Add click-outside handling to overlay --------- Co-authored-by: Chris Wilton-Magras <[email protected]> Co-authored-by: George Sproston <[email protected]> Co-authored-by: Heather Logan <[email protected]>
Description
Based on work by @heatherlogan-scottlogic and @gsproston-scottlogic, this PR tackles some accessibility issues with the overlay.
Highlights:
Supersedes PR #362
Closes #319