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

Fix handling of theme change #505

Merged
merged 7 commits into from
Oct 22, 2024
Merged

Fix handling of theme change #505

merged 7 commits into from
Oct 22, 2024

Conversation

arjxn-py
Copy link
Member

theme.mov

Shall close #480

@arjxn-py arjxn-py added the bug PR that fixes a bug label Oct 20, 2024
Copy link
Contributor

github-actions bot commented Oct 20, 2024

Preview PR at appsharing.space

Copy link
Contributor

github-actions bot commented Oct 20, 2024

Integration tests repot: appsharing.space

@SylvainCorlay
Copy link
Member

A full resize event would trigger a complete reflow of the page. Maybe just doing it on the view would be more economical.

Also, is the resize the only thing that triggers a redraw of the view?

@arjxn-py
Copy link
Member Author

Also, is the resize the only thing that triggers a redraw of the view?

That sounds good, thanks @SylvainCorlay.
I at the moment can't think of any other way but I'm happy to discuss and iterate over it.

@SylvainCorlay
Copy link
Member

Just calling update() on the Lumino panel of the 3-D view should do it. People more familiar with Lumino on the team will know the proper way to do.

@trungleduc
Copy link
Member

you can use MessageLoop.sendMessage to send the Widget.ResizeMessage message to the 3D view widget.

MessageLoop.sendMessage(widget, Widget.Msg.BeforeAttach);

@arjxn-py
Copy link
Member Author

you can use MessageLoop.sendMessage to send the Widget.ResizeMessage message to the 3D view widget.

MessageLoop.sendMessage(widget, Widget.Msg.BeforeAttach);

@trungleduc When i try MessageLoop.sendMessage in model.ts it says

Property 'sendMessage' does not exist on type 
'typeof import("/Users/arjunverma/Desktop/Arjun/JupyterCAD/node_modules/@lumino/messaging/types/index")'

but the same works in formbuilder.tsx

@martinRenou
Copy link
Member

My 2 cents, we used to depend on the lightTheme attribute in the React component state to change the background color according to it: https://github.com/jupytercad/JupyterCAD/blob/main/packages/base/src/3dview/mainview.tsx#L1444

This seems to not be used anymore! So we should probably remove lightTheme from the state, but we should probably modify this _handleThemeChange method there to force a this.update() of the react component to trigger a re-render, instead of setting that old unused state attribute.

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Neat! These are the best PRs

@martinRenou martinRenou changed the title Sync editor on theme change Fix handling of theme change Oct 22, 2024
@arjxn-py arjxn-py merged commit 6f4b7e2 into jupytercad:main Oct 22, 2024
10 checks passed
@arjxn-py arjxn-py deleted the sync-theme branch October 22, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing theme in jupyter cad makes the scene blank
4 participants