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 to data-theme-light to the document body #6554

Closed
ellisonbg opened this issue Jun 12, 2019 · 8 comments
Closed

Move to data-theme-light to the document body #6554

ellisonbg opened this issue Jun 12, 2019 · 8 comments

Comments

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jun 12, 2019

Right now we have a data-theme-light HTML attribute on the div.jp-LabShell that is a child of the document body. Because we attach dialogs to the document body, this theming hint isn't available to style the dialog. The proposed solution is to move this attribute to the document body. There are also other UI elements (completer) that we attach to the body that might need access to this attribute.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 12, 2019

An alternative to this is any component that puts things in the DOM not under the lab shell, it is responsible for working with the theme manager programmatically to inquire and expose the theme information to its DOM elements. This helps us avoid having global state.

For example, what if there are two JLab components running in a page, but not in a single application, each with its own theme manager - who wins the body data spot? In general, I think namespacing is much cleaner.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 13, 2019

To which you would reply, of course: 'Jason, the entire theming system is in the global namespace because all of the CSS variables are in the root namespace. You cannot have two different themes on the page because of this anyway, so you might as well make it official by taking over the body element too.'

Okay, good point. It still feels a bit weird and not very clean, but so is non-namespaced global CSS, and I suppose we can't fix that anytime soon.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 13, 2019

This is set at

app.shell.dataset.themeLight = String(manager.isLight(currentTheme));
app.shell.dataset.themeName = currentTheme;
if (
app.shell.dataset.themeScrollbars !==
String(manager.themeScrollbars(currentTheme))
) {
app.shell.dataset.themeScrollbars = String(
manager.themeScrollbars(currentTheme)
);
}

If we are putting it on the body element, perhaps we should namespace it a bit more than data-theme-light, like at least data-jp-theme-light, etc.? Or maybe even data-jupyterlab-theme-light?

@ian-r-rose
Copy link
Member

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

I agree that namespacing it would make sense. Unfortunately, this is a CSS attribute that was explicitly intended for extension authors to target, so this would be real breaking change to extension theming. But I suppose it's now or never.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 13, 2019

Yes. That's why this is blocking the RC :).

So - was that a vote for namespacing it? What name?

@telamonian
Copy link
Member

@telamonian telamonian commented Jun 13, 2019

Namespacing a body-level attribute seems like a good plan. I vote for data-jp-theme-light, since that's consistent with how all of the CSS vars are namespaced.

@ian-r-rose
Copy link
Member

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

I think data-jp-theme-light is good as well.

@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 pull requests

Successfully merging a pull request may close this issue.

4 participants