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

Expose icon SVG to theme CSS #6034

Merged
merged 85 commits into from Aug 14, 2019
Merged

Conversation

@telamonian
Copy link
Member

@telamonian telamonian commented Feb 25, 2019

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 an svg 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:

image

image

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 25, 2019

Thanks for making a pull request to JupyterLab!

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

1 similar comment
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 25, 2019

Thanks for making a pull request to JupyterLab!

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

@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 25, 2019

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.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 25, 2019

I think this approach looks good!

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 26, 2019

Fantastic, thanks for working on this, it will really help us. A few questions...

  • Right now most of our icons are in the themes. Are you thinking of collecting the icons into a separate new or existing jlab package so lots of other packages can use them?
  • Our current icon set is a mix of Material Design icons, and a few custom ones. Because we have had to manually change the colors of the svg's, we have been manually copying the MD icons into the themes. I would love to migrate over the depending on a well maintained npm package for the MD icons. Do you think this is possible?
  • We are beginning to use blueprint for react components. Have you looked at how they handle icons to make sure this approach will work with this one?

Sorry for all the questions - excited about this direction.

@afshin afshin requested review from ellisonbg and saulshanabrook Mar 27, 2019
@afshin afshin added this to the 1.0 milestone Mar 27, 2019
@ian-r-rose
Copy link
Member

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

@telamonian what is the status of this? It would be great to get it in :)

@gnestor
Copy link
Contributor

@gnestor gnestor commented Apr 18, 2019

@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)!

image

Can I help push this past the finish line?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Apr 29, 2019

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:

  • I do think it would be best to collect all of the icons into a single core package. Right now to me ui-components seems like the best place for them.

  • If you can point me towards such an npm package that has a collection of the MD icons, I can let you know if it's feasible to use. Basically, I think it'll come down to how the package in question exposes it's .svg files.

  • I looked into the blueprint icon stuff. My approach won't interfere with the icons that blueprint provides. I think it may in fact be possible to unify my icon stuff with the existing infrastructure in blueprint. However, I ran into some gnarly build issues when I tried this, so I think that'll have to wait for a future PR.

@telamonian telamonian force-pushed the style-svg-with-css branch from 638a40a to 69eedbb Apr 30, 2019
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented May 2, 2019

Congratulations on defending, Max!

@gnestor
Copy link
Contributor

@gnestor gnestor commented May 5, 2019

Congrats @telamonian!

To respond to your questions:

  • Agreed, I would definitely like to see the icons live here. We only need to keep the custom icons here and then we can use a Material Design icon font or npm package that contains all of the SVGs.

  • You can import the official Material Design icon font

<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.

  • I'm happy to help with the blueprint-related stuff. Blueprint provides a pretty good icon set so we could consider using it.

@saulshanabrook saulshanabrook removed this from the 1.0 milestone May 22, 2019
@saulshanabrook saulshanabrook added this to the 1.1 milestone May 22, 2019
@telamonian telamonian force-pushed the style-svg-with-css branch from e6f5f2d to 28cbedc Jun 4, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Jun 4, 2019

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 IconRegistry stuff (and I'll have to do some more organization in order to make that process simple), but in the current version of this PR most of the tab icon stuff has been automated.

@ian-r-rose
Copy link
Member

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

@telamonian I know some folks at Berkeley who may be interested in hiring a scientist with Jupyter experience 😉

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jun 5, 2019

There are currently two roadblocks to finishing this PR, one simple and one probably quite difficult:

  • I'm having trouble finding a set of colors to use with the icons that a) already exist in the themes and b) reproduce the existing icon colors. All of my icon color CSS is in packages/application/style/images.css, and I added relevant CSS classes directly to the .svg files in packages/ui-components/style.

  • Phosphor handles the creation/rendering of many of jlab's tab icons (eg in the sidebar tabs and topbar document tabs), and does so in an opaque fashion. By overriding some of Phosphor's default VDOM renderers, I've made some headway in getting .svg icons in the tabs. However, my current solutions all rely on a hack whereby the icon nodes are retrieved after their initial creation via document.getElementsByClassName. This is not ideal for a bunch of reasons (including performance). The way I see it, there are two possible development paths:

    1. Figure out how to smoothly integrate IconRegistry with Phosphor. I think this can be done by passing custom VDOM renderers to certain widgets via their constructors. However, I suspect a complete version of this solution will require an upgrade of @phosphor/virtualdom in order to work.
    2. Work on polishing up my current hacks to ensure they're robust and do the minimum required work.

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.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jun 9, 2019

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:

  • Tools for adding inline svg icons, mostly encapsulated in the IconRegistry class. All of this stuff has been designed to play nicely with the existing icon handling (as css background images), so both can co-exist.

  • All existing .svg files in core (including all of the left sidebar tab icons, excluding the hundred-or-so remaining icons in the theme packages) have been moved to ui-components, and are being handled by IconRegistry. I've added classes (and associated CSS) to the elements of .svg files so they repaint correctly in accordance with the theme.

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.

@telamonian telamonian force-pushed the style-svg-with-css branch 2 times, most recently from d4a8d6c to 57859d6 Jun 14, 2019
@telamonian telamonian closed this Jun 14, 2019
@telamonian telamonian reopened this Jun 14, 2019
@tslaton
Copy link
Collaborator

@tslaton tslaton commented Jun 26, 2019

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).

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jun 26, 2019

@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.

@telamonian telamonian force-pushed the style-svg-with-css branch from 1e8d251 to 48d6cec Aug 14, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 14, 2019

Thanks for your hard work and patience on this, @telamonian!

@blink1073 blink1073 merged commit 083c65d into jupyterlab:master Aug 14, 2019
7 of 9 checks passed
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Aug 14, 2019

Thanks for the herculean effort @telamonian!

@telamonian
Copy link
Member Author

@telamonian telamonian commented Aug 14, 2019

I'm just very happy right now that I will never have to rebase this PR ever again. Thanks guys.

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.

None yet

10 participants