-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Conversation
…xt menu Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
There was a problem hiding this 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>
There was a problem hiding this 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.
Oh overlooked that. good chatch im gonna take another look |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@msujew not rendering when too small and the maximum update deapth should be fixed now. |
@jonah-iden Linting fails in the CI. |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
fixed now |
There was a problem hiding this 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
weird. It worked fine for me. I'll take another look thanks for testing |
@msujew are you sure you build it correctly? Aufzeichnung.2024-04-30.154816-theia-notebook-toolbar-resize.mp4For me it looks like this |
@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. |
oh ok |
Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
now it should finally work fine |
There was a problem hiding this 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 👍
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 itemsHow 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
MenuAction
s like in vscodeReview checklist
Reminder for reviewers