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

Make themes dynamic and allow extensions to specify a module path #2759

Merged
merged 52 commits into from Aug 7, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 27, 2017

Fixes #2757. Partially addresses #1084.

  • Allows extensions to specify a module path to load.
  • Makes themes dynamically loaded and determined by a user setting.
  • Adds documentation for settings and theme authoring.
  • Dissolves @jupyterlab/theming into @jupyterlab/application and the two theme packages as appropriate.
  • Adds more automation in the package integrity check and the creation of the jupyterlab/package.json file.

theme-switch

@@ -66,10 +68,3 @@ var indexPath = path.join(basePath, 'packages', 'all-packages', 'src', 'index.ts
var index = fs.readFileSync(indexPath, 'utf8');
index = index + 'import "' + package.name + '";\n';
fs.writeFileSync(indexPath, index);

// Add the extension to jupyterlab/package.json
var jupyterlabPackagePath = path.join(basePath, 'jupyterlab', 'package.json');
Copy link
Member

@ian-r-rose ian-r-rose Jul 31, 2017

Choose a reason for hiding this comment

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

I confess to still being unclear on the intention of jupyterlab/package.json vs jupyterlab/package.app.json. If this is removed, does it need to be added elsewhere for the python package to pick up the new sibling?

In any event, a change here should be mirrored in scripts/remove-sibling.js.

Copy link
Member Author

@blink1073 blink1073 Jul 31, 2017

Choose a reason for hiding this comment

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

Thanks for the heads up. package.json and package.app.json are mean for dev mode and app mode, respectively. The latter is copy of package.json at the time of publishing the Python package.

Copy link
Member

@ian-r-rose ian-r-rose Jul 31, 2017

Choose a reason for hiding this comment

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

So they are autogenerated (obviating the need to patch it in this script)?.

Copy link
Member Author

@blink1073 blink1073 Jul 31, 2017

Choose a reason for hiding this comment

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

package.app.json was already auto-generated, this PR adds auto-generation for package.json as well.

Copy link
Member

@ian-r-rose ian-r-rose Jul 31, 2017

Choose a reason for hiding this comment

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

Ah, makes sense, thanks.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jul 31, 2017

Plan for handling of themes:

  • Add optional themeCSS metadata that is a path to a CSS file that is loaded as a string (not automatically added to the DOM).
  • Add a new theme manager plugin has the following API:
class ThemeManager
    // The themeCSS is populated in the webpack entry point
    constructor({ settingRegistry, themeCSS });
    theme: string;
    setTheme(name): Promise<void>;
    // Used for a theme picker
    themes: ReadonlyArray<string>;  
    register(theme: ITheme);
}

interface ITheme {
    name: string;
    displayName: string;
    load(): Promise<void>;
    unload(): Promise<void>
}
  • A theme is loaded by
    • Unloading the previous theme (if applicable), and removing its CSS from the private style tag
    • Adding the new CSS to the private style tag, and then loading the new theme
  • The current theme is linked to a setting in the theme manager plugin

In this way, we meet the following goals:

  • The initial theme CSS can be loaded synchronously during app startup, preventing a jarring change in fonts/colors
  • Themes are discoverable and can be changed dynamically without page reload
  • The theme author does not need to use any WebPack-specific syntax

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 1, 2017

We can't switch themes dynamically because we can't load additional CSS that is
removable without exposing the loading pattern (let themeCSS = require('!css-loader!foo.css').toString()).

So, a themeExtension is a standard application extension, but has the following metadata that allows us to discover and load only the appropriate theme plugin:

jupyterlab: {
    themeExtension: {
       name: 'JupyterLab Dark',
       path?: ''
    }
}

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 1, 2017

A plan that allows for dynamic loading:

  • Add a /themes folder to the application that has the static theme assets, copied when the extension is installed.
  • Add an api/themes endpoint to serve these files.
  • The client side theme manager has a loadCSS(path: string) method that makes a request to the api/themes endpoint for the asset and adds it to a script tag. It maintains a disposable set that removes any script tags added by the current theme.
  • We can move the light and dark theme icons to their respective themes, and they will not be a part of the main bundle.

The downside is that any assets used from third party libs must be included in the package (e.g. icon files), since we will not have WebPack to resolve the dependency paths.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 1, 2017

Proof of concept of the above done, loading a theme dynamically using a link tag. The only additional change made is that relative url() specifiers in CSS need to include the appropriate static link, e.g.: --jp-image-jupyter: url('./lab/api/themes/jupyterlab-theme-light-extension/style/images/jupyter.svg');, otherwise they are loaded relative to the page url.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 2, 2017

Progress:

theme-switch

@blink1073 blink1073 changed the title Allow extensions to specify a module path Make themes dynamic and allow extensions to specify a module path Aug 2, 2017
@blink1073 blink1073 force-pushed the path-to-extension-2 branch from f0246a3 to fad1fd7 Aug 3, 2017
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 3, 2017

afshin
afshin approved these changes Aug 4, 2017
Copy link
Member

@afshin afshin left a comment

This is awesome! Thanks for slogging through it.

}

const plugin: JupyterLabPlugin<void> = {
id: 'theme-light-extension',
Copy link
Member

@afshin afshin Aug 4, 2017

Choose a reason for hiding this comment

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

What is the rationale for not calling these jupyter.themes.light or 'jupyter.extensions.light-theme' or some variety of jupyter.foo.bar?

@blink1073 blink1073 merged commit 4b654f3 into jupyterlab:master Aug 7, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Aug 7, 2017
@blink1073 blink1073 deleted the path-to-extension-2 branch Aug 7, 2017
@santiagobasulto
Copy link

@santiagobasulto santiagobasulto commented Feb 4, 2019

Hello people. I was reading this PR trying to understand where can I set the default theme for all my sessions. Is there any file I could write in my $HOME that will read automatically the settings? So I can leave the Dark template set by default every time I run the lab? Thanks!

@pisymbol
Copy link

@pisymbol pisymbol commented Apr 16, 2019

Is there any configuration file that would allow me to start jupyterlab with the dark theme as the default? This seems like an obvious and good feature to have.

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 17, 2019

@pisymbol Thanks for your feedback! Please open a new issue about this, to increase visibility for the feature request, and to ensure it doesn't simply disappear (comments on merged PRs will not be picked up during triage, and it doesn't make sense to reopen a PR).

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 17, 2019

PS: If your reason for not directly opening an issue was that you were unsure if such functionality already existed, discourse might be a good alternative: https://discourse.jupyter.org/c/jupyterlab

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 17, 2019

If you change the theme, it should be saved and start with that theme. Is that what you are asking?

@pisymbol
Copy link

@pisymbol pisymbol commented Apr 17, 2019

I'm saying I should be able to write out a json config parameter somewhere that specifies what theme I want on bootup (the default theme - in this case it maybe just light or dark but I can imagine people will want to specify their own theme).

Think about for example setting up a docker container with JuptyerLab in it and in the Dockerfile I want to be able to pre-config the dark theme as my default theme by writing out some config file in the container's user's home directory. Optionally, there could also be a command line argument to change the initial theme for the current session. Best of both worlds.

Does that make sense?

And yes, you should be able to save the last theme as the default one in general. It's really annoying that I have to set it to dark every single time I start JupyterLab.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 17, 2019

@pisymbol you can distribute a settings JSON file that specifies the default theme with your docker container. The last-set theme is already saved in the settings system, so that should already persist between launches of the application.
An example of this can be found here: https://github.com/ian-r-rose/binder-workspace-demo

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 17, 2019

And there is actually one more place settings can be overridden, between the defaults specified in the plugin and the user settings: the application-level default settings. That may be a more appropriate place for overriding settings without touching the user settings files.

See the last paragraph of the extension setting section at https://jupyterlab.readthedocs.io/en/latest/developer/extension_dev.html#extension-settings

@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 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.

7 participants