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
Expose icon SVG to theme CSS #6034
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
1 similar comment
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
I figure I'll wrap up work on this PR once I've finished exposing the remaining non-theme icons (there' 3 or 4 left). I would like to get some feedback at this point. This is my first deep(-ish) dive into the webpack stuff (not to mention the first time I've even touched any of the React stuff). If anyone has thoughts about how I could be doing something simpler/better, I'd love to hear them. |
I think this approach looks good! |
Fantastic, thanks for working on this, it will really help us. A few questions...
Sorry for all the questions - excited about this direction. |
@telamonian what is the status of this? It would be great to get it in :) |
@telamonian We are getting very close to a 1.0 release and I agree that this would be good to get in there! If we don't merge this, then we will need to make changes to individual extensions because I'm noticing that many don't support dark themes (or don't match the other core icons)! Can I help push this past the finish line? |
I took some time to finish defending my thesis, and then I was dead to the world for a while, but I'm back on this now. @ellisonbg I have answers to your questions now:
|
638a40a
to
69eedbb
Compare
Congratulations on defending, Max! |
Congrats @telamonian! To respond to your questions:
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons"> If you need the SVG files, I don't immediately see an npm package that provides them, but they're all in https://github.com/google/material-design-icons.
|
e6f5f2d
to
28cbedc
Compare
Apologies for the delay. Partly I've been busy (now that I'm no longer a student, I need to find paying work), and partly I ran head first into a bunch of issues with Phosphor, and how it handles icons. Good news, though @gnestor! I fished your wish, and now the tab icons on the sidebar are all themable. Extensions devs will have to set up their icons via my |
@telamonian I know some folks at Berkeley who may be interested in hiring a scientist with Jupyter experience 😉 |
There are currently two roadblocks to finishing this PR, one simple and one probably quite difficult:
I'll keep working on it. @gnestor @ian-r-rose (or anyone) If I could get a hand working out these last few issues it would be greatly appreciated. Also, Ian, that sounds interesting. Possibly very interesting. I'll email you to follow up. |
I figured out some reasonable ways to deal with the phosphor issues (via some light subclassing). Functionally, I think this PR is done. Here's what the PR covers:
There are still a number of issues, but they largely all have to do with integrating the inline svg icon stuff better with core and extensions. Right now seems like a good time to get some further review/feedback. @saulshanabrook @blink1073 @ellisonbg @ian-r-rose @gnestor any input you can give me would be greatly appreciated. |
d4a8d6c
to
57859d6
Compare
I was looking into adding icons matching the style of JupyterLab core to an extension I'm working on, and upon struggling to style those icons, I came across this issue. I think the progress sounds good! Since this is a large rework of all the icons anyway, I wonder: are they licensed correctly currently? Google says their Material Design icons are licensed under Apache 2.0, but I don't see the license included anywhere. I was wondering how to include it myself. Curiously on their own copy of the license, they appear to have failed to register their copyright (google/material-design-icons#912). |
@tslaton I can honestly say that I have no idea whatsoever. But if you want to dig into it I'd be glad to give you a hand. I'll probably save it for the next PR, though. This one has already creeped all of the scopes. |
also added a better failure mode to IconRegistry.iconReact
the `ensureUiComponents` function in buildutils ensures that the imports in iconImports.ts are synced with the contents of ui-components/style/icons. This runs as part of `jlpm integrity`. I also improved some docs
1e8d251
to
48d6cec
Compare
Thanks for your hard work and patience on this, @telamonian! |
Thanks for the herculean effort @telamonian! |
I'm just very happy right now that I will never have to rebase this PR ever again. Thanks guys. |
As per @ian-r-rose's suggestion in #5960.
Short term, this PR will make it easier to add new icons in core; there will never be a need to make 2+ copies of each icon for light, dark, pink, etc, themes, like in the current version of
statusbar
.Medium term, this will enable us to automatically provide a complete set of themed icons. In in turn, this will free theme devs from having to manually repaint/manage the 100+ icons currently in a theme.
Details
In order to re-theme an icon using CSS, the icon's
*.svg
first has to be added to the DOM as an actual element. Our current CSS-only setup for importing SVGs isn't able to do this (I guess CSS by itself is never able to make those kinds of changes to the DOM; the closest it gets are non-stylable replaced elements). So my goal is to add two things to Jupyterlab:A means by which to directly import
*.svg
files into*.ts
(WIP) and*.tsx
(done) files. This requires ansvg
specific.d.ts
file, and some tweaks to the webpack loaders.A collection of objects/functions for attaching imported
svg
to the DOM. Ideally, these will be even simpler to use than the current CSS handling (which requires ~5 lines for each icon).At the present moment, I've finished implementing all of this stuff for the
*.tsx
files in@jupyterlab/statusbar
. I've removed the duplicate icons from this package, and have gotten all of its icons to play nicely with the different themes: