Skip to content
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

Add "Skip to content" button #5773

Closed
wants to merge 48 commits into from
Closed

Add "Skip to content" button #5773

wants to merge 48 commits into from

Conversation

andrico1234
Copy link
Contributor

@andrico1234 andrico1234 commented Jan 14, 2021

Description

I've added a button inside of Layout.tsx that only displays when the first action a user performs on a RA site is pressing the tab button.

This moves the focus into the main content of the website, avoiding having to tab through the top and side menus.

While the functionality is complete, I'm going to write Cypress tests for this functionality.

Notes:

Because navigating to anchors via the # didn't seem to work nicely with HashRouter, I had to install an node module that exports a custom link. Here's a link to the discussion

Fortunately the module has no dependencies, and it works out of the box.

I implementing this component similarly to how it's written inside of the Material-UI repo. You can view their implementation here

skip-navigation-smol.mov

fzaninotto and others added 30 commits November 27, 2020 17:20
Change Notification's error color from dark to main
Allow MenuItemLink to receive TooltipPops
@fzaninotto
Copy link
Member

Awesome!

I'm a bit concerned by the fact that https://github.com/rafgraph/react-router-hash-link only works with BrowserRouter. And I think that, since you know the target element is not focusable (it's a div), you can replicate react-router-hash-link's focus management inside an onClick in the button without using a Link. This would a allow to remove the dependency.

@andrico1234
Copy link
Contributor Author

@fzaninotto

Good shout, I managed to get rid of the hash-link package by mimicking their implementation. If no element is found from the getElementById function, then I just do nothing. I imagine this would only happen if someone passes through custom routes. What should the expected behaviour be in this scenario, a simple console.warn?

@djhi
Copy link
Collaborator

djhi commented Jan 15, 2021

What should the expected behaviour be in this scenario, a simple console.warn?

Yes and only in development mode please.

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@djhi
Copy link
Collaborator

djhi commented Jan 18, 2021

What's missing? The PR title is still prefixed with [WIP]

@andrico1234
Copy link
Contributor Author

I had trouble in the Cypress test. Specifically when it came to determining the focus position after clicking 'Skip to content' button. It seems that because divs don't have focus, I can't use the cy.focused() to check to see if #main-content has focus.

Other than that everything else is done. I'll leave it with you folks to determine whether enough's here to merge in, or whether you want a more comprehensive test included.

@andrico1234 andrico1234 changed the title [WIP] Add "Skip to content" button Add "Skip to content" button Jan 18, 2021
@fzaninotto
Copy link
Member

Do you mind rebasing on the latest commit of the next branch?

@andrico1234
Copy link
Contributor Author

I fudged the rebase a little! I'll try and revert

@andrico1234
Copy link
Contributor Author

closing in favour of: #5804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants