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 theme dataset attributes to the body element and namespace them #6566

Merged
merged 4 commits into from Jun 14, 2019

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 13, 2019

References

Fixes #6554

Code changes

This moves the theme variables to the document.body element from the app.shell element.

I also audited where we are doing things with document.body, and as a result this also moves several other attributes from document.body to the app.shell element.

User-facing changes

Backwards-incompatible changes

The theme dataset variables change names.

jasongrout added 2 commits Jun 13, 2019
This namespaces the dataset attributes, so is a breaking change for users using these names in their CSS styles.
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 13, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 13, 2019

@ian-r-rose - the code to move widgets between sidebars seems to use a body attribute solely as a way of having global state. Can we change the code to not use the body element as a place to hold global state? Right now this PR is a little broken, in that I just commented out relevant code there.

@jasongrout jasongrout changed the title Themevar Move theme variables to the body element Jun 13, 2019
@jasongrout jasongrout changed the title Move theme variables to the body element Move theme dataset attributes to the body element Jun 13, 2019
@jasongrout jasongrout changed the title Move theme dataset attributes to the body element Move theme dataset attributes to the body element and namespace them Jun 13, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 13, 2019

The sidebar dataset was mostly not used (and has maybe been in lab for a long time) -- I think that the usage you pointed to was just a fallback in case the context-menu-widget-finding failed.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

I pushed a change with some cleanup @jasongrout. I'm happy with this if you are.

@ian-r-rose ian-r-rose added this to the 1.0 milestone Jun 13, 2019
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Jun 14, 2019

Thanks! This looks good to me too.

@jasongrout jasongrout merged commit 5862b2e into jupyterlab:master Jun 14, 2019
9 checks passed
@jasongrout jasongrout mentioned this pull request Jun 14, 2019
@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants