-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Close launcher button styling and close launcher with delete key #655
base: main
Are you sure you want to change the base?
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
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 added some review comments but before we can move forward with this PR we have to remove all the code-formatting changes that were introduced.
Also, please update the PR description. It's currently a (near) duplicate of jupyterlab/jupyterlab#15347 |
removed launcher caption and button padding
Review comments addressed; not approving yet as waiting for CI but no longer blocking in case if someone else gets back to it before me.
@gabalafou I see that you mentioned this PR in jupyterlab/jupyterlab#15347. Once this is ready I am happy to merge and release but currently your review is blocking here: |
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 still have some concerns and questions. Will get back to this asap.
tab.contains(focusedElement) | ||
); | ||
if (index >= 0) { | ||
this.currentIndex = index; |
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.
Why is this here?
{ | ||
className: 'lm-TabBar-tabLabel', | ||
tabindex: '-1', | ||
'aria-hidden': 'true' |
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.
Why is this aria-hidden: true
?
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.
oops meant to mark it request changes
References
#15303 - Issue no. 3 - Arrow key navigation between launcher tabs not as expected
#15347 - Change close launcher div to button for screen reader accessibility
When screen reader is active, pressing caps lock + right arrow key lands on close launcher div but does not close on Enter.
Accessibility documentation states an optional keyboard interaction where, if deletion is allowed, pressing "Delete" closes the current tab element and its associated tab panel
Code changes
Changed styling of close launcher to original after div has been changed to button (in PR 15347)
Enabling focused tab to be closed using "Delete" on keyboard