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

Clean up interface switcher plugin in preparation for release #6766

Merged
merged 8 commits into from
Mar 14, 2023

Conversation

afshin
Copy link
Member

@afshin afshin commented Mar 13, 2023

This PR makes the INotebookTracker required instead of checking for it at runtime. It also adds an ellipsis character to the interface button/dropdown to indicate it is a two-step UI control.

@afshin afshin added this to the 7.0 milestone Mar 13, 2023
@afshin afshin self-assigned this Mar 13, 2023
@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch afshin/notebook/interface-switcher

Copy link
Member Author

@afshin afshin left a comment

Choose a reason for hiding this comment

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

bot please update playwright snapshots

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

Thanks @afshin.

It also adds an ellipsis character to the interface button/dropdown to indicate it is a two-step UI control.

Maybe we could use the same caret icon as in the file browser for the "New" dropdown?

const menubar = new MenuBar(overflowOptions);
const newMenu = new Menu({ commands });
newMenu.title.label = trans.__('New');
newMenu.title.icon = caretDownIcon;

image

@afshin
Copy link
Member Author

afshin commented Mar 13, 2023

That's a good idea for consistency.

@afshin
Copy link
Member Author

afshin commented Mar 13, 2023

bot please update playwright snapshots

@jtpio jtpio closed this Mar 13, 2023
@jtpio jtpio reopened this Mar 13, 2023
@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

Kicking CI.

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

The snapshot update shows that the label is now slightly higher:

image

Maybe we could try to re-align it like it was before, but this could also be done in a follow-up PR.

Copy link
Member Author

@afshin afshin left a comment

Choose a reason for hiding this comment

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

bot please update playwright snapshots

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

Looks like there were no changes to push when updating the snapshots? https://github.com/jupyter/notebook/actions/runs/4405807628/jobs/7717182164

image

@afshin
Copy link
Member Author

afshin commented Mar 13, 2023

Chromium fails but Firefox passes ... do the snapshots only come from Firefox?

I visually can confirm my changes are visible in Chrome.

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

do the snapshots only come from Firefox?

No it's for both chromium and firefox. We can try again.

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

bot please update playwright snapshots

@jtpio
Copy link
Member

jtpio commented Mar 13, 2023

I visually can confirm my changes are visible in Chrome.

hmm the snapshot update still doesn't pick them up. I also tried building this branch in a fresh environment and this is what it looks like in Firefox (not aligned with the kernel name):

image

Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
@afshin
Copy link
Member Author

afshin commented Mar 14, 2023

bot please update playwright snapshots

@jtpio jtpio closed this Mar 14, 2023
@jtpio jtpio reopened this Mar 14, 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!

@jtpio jtpio merged commit f44ac97 into jupyter:main Mar 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 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.

None yet

2 participants