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

Close launcher button styling and close launcher with delete key #655

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

e218736
Copy link

@e218736 e218736 commented Nov 2, 2023

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

Copy link

welcome bot commented Nov 2, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link
Contributor

@gabalafou gabalafou left a 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.

packages/widgets/src/tabbar.ts Outdated Show resolved Hide resolved
packages/widgets/src/tabbar.ts Outdated Show resolved Hide resolved
packages/widgets/src/tabbar.ts Outdated Show resolved Hide resolved
@gabalafou
Copy link
Contributor

Also, please update the PR description. It's currently a (near) duplicate of jupyterlab/jupyterlab#15347

@e218736 e218736 marked this pull request as ready for review November 13, 2023 14:56
@e218736 e218736 changed the title Close launcher keyboard navigation/shortcut Close launcher button styling and close launcher with delete key Nov 17, 2023
packages/widgets/src/tabbar.ts Outdated Show resolved Hide resolved
packages/widgets/src/tabbar.ts Show resolved Hide resolved
@krassowski krassowski added enhancement New feature or request accessibility Addresses accessibility needs labels Dec 4, 2023
@krassowski krassowski dismissed their stale review December 4, 2023 17:59

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.

@krassowski
Copy link
Member

@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:

image

Copy link
Contributor

@gabalafou gabalafou left a 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;
Copy link
Contributor

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'
Copy link
Contributor

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?

Copy link
Contributor

@gabalafou gabalafou left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Addresses accessibility needs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants