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

Use responsive design for the main notebook toolbar #13663

Merged
merged 5 commits into from Apr 30, 2024

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Apr 29, 2024

What it does

When the notebook editor widget in not wide enough to contain all main toolbar items it will now add a ... menu containing all overflowing items

How to test

Open a notebook and adjust the widget size. You should see groups being added and removed again from the context menu.

Follow-ups

Currently only the top level groups are added to context menu instead of singular MenuActions like in vscode

Review checklist

Reminder for reviewers

…xt menu

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden jonah-iden requested a review from msujew April 29, 2024 13:01
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Works pretty well in general, but we have a reference/memory leak due to ResizeObserver in there.

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

That's better, but I've noticed that with this change there is a minor regression: When resizing the window to be "too small" for anything to be displayed in the widget, the widget just renders as empty:

2024-04-29.16-56-35.mp4

I'm also getting this error in the console:

Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.

@jonah-iden
Copy link
Contributor Author

Oh overlooked that. good chatch im gonna take another look

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden
Copy link
Contributor Author

jonah-iden commented Apr 30, 2024

@msujew not rendering when too small and the maximum update deapth should be fixed now.

@msujew
Copy link
Member

msujew commented Apr 30, 2024

@jonah-iden Linting fails in the CI.

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@msujew msujew changed the title Main toolbar adjusting to maximum size by putting items in to a context menu Use responsive design for the main notebook toolbar Apr 30, 2024
@msujew msujew added ui/ux issues related to user interface / user experience notebook issues related to notebooks labels Apr 30, 2024
@jonah-iden
Copy link
Contributor Author

fixed now

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

With the newest change it looks like the amount of displayed groups can only increase/decrease by one. However, this isn't the expected behavior when minimizing/maximizing the application:

2024-04-30.15-13-04.mp4

@jonah-iden
Copy link
Contributor Author

weird. It worked fine for me. I'll take another look thanks for testing

@jonah-iden
Copy link
Contributor Author

@msujew are you sure you build it correctly?

Aufzeichnung.2024-04-30.154816-theia-notebook-toolbar-resize.mp4

For me it looks like this

@msujew
Copy link
Member

msujew commented Apr 30, 2024

@jonah-iden slowly resizing the app works for me as well. The issue comes up when the resizing is happening in a single discrete step, like when maximizing the window (as shown in my video). In this case this code will only trigger once and unhide a single group in the toolbar.

@jonah-iden
Copy link
Contributor Author

oh ok

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden
Copy link
Contributor Author

now it should finally work fine

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thank you, everything works well now 👍

@jonah-iden jonah-iden merged commit b20a751 into master Apr 30, 2024
14 checks passed
@jonah-iden jonah-iden deleted the jiden/dynamic-notebook-main-toolbar branch April 30, 2024 15:53
@github-actions github-actions bot added this to the 1.50.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook issues related to notebooks ui/ux issues related to user interface / user experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants