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

Bump to JupyterLab 4.1.0a4 bis #7172

Merged
merged 13 commits into from
Dec 4, 2023
Merged

Conversation

brichet
Copy link
Collaborator

@brichet brichet commented Dec 2, 2023

This PR supersede #7166

Changes

  1. CSS in toolbar to reflect the changes on toolbar components, following [Accessibility] Using arrow keys to navigate in toolbars items jupyterlab/jupyterlab#15021

  2. Filebrowser toolbar

    See Bump to JupyterLab 4.1.0a4 #7166 (comment) for context.

    It changes the logic to include the action buttons in the filebrowser toolbar, and restore the previous appearance of the action buttons.

    The action button in the filebrowser toolbar were included in an actions toolbar, which breaks keyboard-only usage (using tab and arrow keys).

    With this PR, the buttons are included in the file browser toolbar as direct child.

    NOTES:

    • the buttons are now hidden when not usable, instead of being not rendered. This means that a user can display these buttons from the DOM. I don't think this is critical.
    • it modifies the logic to hide or not the rename button. I didn't really understand the previous logic, now it's displayed if a single item is selected. Let me know if I should revert this change.

Copy link
Contributor

github-actions bot commented Dec 2, 2023

Binder 👈 Launch a Binder on branch brichet/notebook/filebrowser-toolbar

@brichet
Copy link
Collaborator Author

brichet commented Dec 2, 2023

@jtpio I was not able to open this PR in your branch https://github.com/jtpio/notebook/tree/update-lab-410a4
Feel free to change the target branch to make it more easier.

I also don't know why there are conflicts here and not in #7166, since the conflicting files have been changed there...

@jtpio
Copy link
Member

jtpio commented Dec 2, 2023

No problem, we can close #7166 in favor of this one.

There are also some conflicts on #7166, likely related to the merge of #6968:

image

@jtpio jtpio added this to the 7.1 milestone Dec 2, 2023
@brichet brichet force-pushed the filebrowser-toolbar branch from 763111e to 7e1bbc0 Compare December 3, 2023 20:14
@brichet brichet changed the title Filebrowser toolbar - actions buttons Bump to JupyterLab 4.1.0a4 bis Dec 3, 2023
@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

Thanks @brichet for picking this up!

2. it modifies the logic to hide or not the rename button. I didn't really understand the previous logic, now it's displayed if a single item is selected. Let me know if I should revert this change.

It was mimicking the behavior of the classic notebook. With some buttons available depending on the number being selected and their type. Normally the rename button was already rendered when a single item was selected:

image

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

Trying on Binder, it looks like the text content of the delete button is not visible by default:

notebook-delete-button.webm

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

Also we might need to tweak the CSS a bit so the hover button background takes the full height on hover:

notebook-hover-buttons.webm

Currently with 7.0.6 only the upload and refresh buttons have a small gap on hover:

fileactions-buttons-hover.webm

We can also fix that separately, maybe it was introduced before in #7161.

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

It was mimicking the behavior of the classic notebook. With some buttons available depending on the number being selected and their type. Normally the rename button was already rendered when a single item was selected:

Testing it again, it depends of the selected files. The test for multiple files was on type === 'file', but notebook files for instance has type='notebook'. The rename button was still there if several notebook files were selected, a soon as there is no directory or
a single file other than notebook.

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

Trying on Binder, it looks like the text content of the delete button is not visible by default:

I can't reproduce it, the delete button is red on my side:

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

I can't reproduce it, the delete button is red on my side:

On Binder, or locally?

Here is what it looks like on Firefox:

image

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

On Binder, or locally?

Here is what it looks like on Firefox:

I tested it on chrome.
You're right, the delete button is white on Firefox, locally or with binder.

We should probably add a snapshot including the delete button.

display: none;
}

.jp-FileActions .jp-ToolbarButtonComponent[data-command='filebrowser:delete'] {
.jp-FileBrowser-toolbar
.jp-FileAction:has(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this might be the cause for the Firefox rendering issue? (with Firefox 118)

https://caniuse.com/css-has

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, this is indeed the issue.

We should probably use the ID of element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be fixed now, using the ID

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

Also we might need to tweak the CSS a bit so the hover button background takes the full height on hover

Fixed in b7f24c6

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

Nice thanks @brichet.

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

bot please update playwright snapshots

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

bot please update playwright snapshots

I added a CSS rule for the 'New' menu (the text was not centered in the component).
I believe we'll have to run it again.

@jtpio
Copy link
Member

jtpio commented Dec 4, 2023

Looks like the bot picked up the comment from the quoted text :)

image

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

Looks like the bot picked up the comment from the quoted text :)

image

Yes, just seen the 👍 icon in my comment.

@brichet
Copy link
Collaborator Author

brichet commented Dec 4, 2023

Kicking CI

@brichet brichet closed this Dec 4, 2023
@brichet brichet reopened this Dec 4, 2023
Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks @brichet!

@jtpio jtpio mentioned this pull request Dec 4, 2023
@jtpio jtpio merged commit a74cd91 into jupyter:main Dec 4, 2023
31 checks passed
@brichet brichet deleted the filebrowser-toolbar branch December 4, 2023 14:57
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants