-
Notifications
You must be signed in to change notification settings - Fork 12
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
319 accessibility and narrator issues for overlay #362
319 accessibility and narrator issues for overlay #362
Conversation
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.
Works good for me!
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.
One change requested - React refs FTW!
@@ -75,12 +78,14 @@ function App({ isNewUser }: { isNewUser: boolean }) { | |||
}, [currentLevel]); | |||
|
|||
function closeOverlay() { | |||
setShowOverlay(false); | |||
const modalDialog = document.querySelector("dialog"); |
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.
As before, if you find yourself querying elements from document
, you're not doing it React style.
In this case, you could use a forward ref on the HandbookOverlay component. However, it's probably far simpler to pass a prop to HandbookOverlay telling it whether we want it to be open or not, and then use a state var in this App component to store and update that state on clicking the icon.
const [overlayVisible, setOverlayVisible] = useState(false);
function openHandbook() {
setOverlayType(OVERLAY_TYPE.HANDBOOK);
setOverlayVisible(true);
}
function closeOverlay() {
setOverlayVisible(false);
}
<HandbookOverlay {...{ currentLevel, overlayType, closeOverlay }} isOpen={overlayVisible} />
That's not the end of the story though, as you'll need to use the dialog.showModal() function inside HandbookOverlay component, in order to get a modal popup on-demand. So you'll still need a standard React ref to grab a DOM reference to the dialog element. You'll need to use that ref in a useEffect, to fire showModal or closeModal whenever isOpen changes its value.
X | ||
</button> | ||
<div className="handbook-overlay-content">{showOverlayByType()}</div> | ||
<dialog className="handbook-overlay" onClick={closeOverlay}> |
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 do wonder if we should be binding the click to the window, instead of directly to the dialog element, and then determining from the click event whether the click was outside the content box. Then you'd likely not need to disable those linter rules. I'm going to check out your branch and see...
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.
Yes that works 🎉
Code in Heather's overlay branch has moved things on significantly, so this will need reworking. I'm going to grab Doro and pair on that, to transfer the changes plus suggested improvements onto another branch based on Heather's changes, then push up a new PR. |
Superseded by #391, closing this PR. |
Changed the overlay to use the html dialog element. This allows for esc on close, and better focus for keyboard users.
I had to disable a few a11y rules in HandbookOverlay.tsx to allow the user to click outside of the dialog to close it.
I think this is fine as we allow for other ways to close the dialog (esc, close button).
I've added a comment to explain why we're disabling those rules.