-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 Link #10126
Add Skip Link #10126
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
This looks done from my perspective! I think this is a good place to fulfill skiplink needs and for us to build off of in having a more robust set of skiplinks by region (as discussed in accessibility meetings) once we know this is working. Wonderful work on and bravely diving into a new topic and asking thoughtful questions (while teaching us all more about skiplink in the process)! |
Side note: JupyterLab uses a |
🤦 Removed. |
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! From a basic coding and layout perspective, this looks good. I'd like to get confirmation from @manfromjupyter that this skip link does what it's supposed to do before we pull it in
removed newline noise
|
||
private _focusFileSearch() { | ||
const input = document.querySelector( | ||
'#filename-searcher .bp3-input' |
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.
'#filename-searcher .bp3-input' | |
'#filename-searcher input' |
.bp3-input
is a styling class that may change in the future. Searching for just the input
tag will make this less fragile
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.
Trying this PR out on binder, it seems like in fact it's currently not quite doing everything a skip link needs to be doing. We should discuss this more at the upcoming accessibility meeting.
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 (for real this time)! I tested this out on my own machine. There are still some issues (eg pressing tab multiple times when a notebook is open will end up indenting the first cell of the notebook and never reach the skip link), but basic functionality is there and we can iterate and improve on this
Thanks! |
References
#9688
Code changes
User-facing changes
Screen.Recording.2021-04-27.at.11.01.05.PM.mov
Backwards-incompatible changes