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

Move MenuBar index check to an overridable method #733

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

Conversation

sdirix
Copy link

@sdirix sdirix commented Dec 20, 2024

Added a protected isValidIndex method to the MenuBar class to handle index validation before setting an index.

This enables adopters to customize the validation logic, e.g., for supporting dynamic menus that may initially be empty.

Resolves #729

Added a protected isValidIndex method to the MenuBar class to handle
index validation before setting an index.

This enables adopters to customize the validation logic, e.g., for
supporting dynamic menus that may initially be empty.

Resolves jupyterlab#729
@sdirix
Copy link
Author

sdirix commented Dec 20, 2024

Let me know whether I can do anything to help resolve the errors in the CI

@krassowski krassowski added the enhancement New feature or request label Dec 20, 2024
@sdirix
Copy link
Author

sdirix commented Dec 20, 2024

@krassowski I pushed the linter fix as a separate commit. If you prefer squashed commits, I can also happily do that

@krassowski
Copy link
Member

Separate commits work well, we can squash on merge.

To fix Does PR have API changes? / api-changes you will need to refresh review/api/widgets.api.md by running yarn run api and committing changes.

I am not sure why Fix License Headers / header-license-fix is failing, it appears unrelated.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you @sdirix!

@sdirix
Copy link
Author

sdirix commented Dec 20, 2024

@krassowski Please wait with the merge. While this fix enables us to build our menus, I just realized that there are some usability edge cases remaining, e.g. the menus can be opened but do not react on hover.

I had a quick glance at the code base: There are more places in which items.length are checked, which could be the culprit, so this PR is overall incomplete. If we settle on a different approach to tackle the issues, then the menubar was "enhanced" without a real benefit to anyone.

I'll come back to you. Sorry for the inconvenience!

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

Successfully merging this pull request may close these issues.

Allow empty menus to activate in menu bar
2 participants