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

Jlicon migration #7700

Merged
merged 102 commits into from Jan 7, 2020
Merged

Jlicon migration #7700

merged 102 commits into from Jan 7, 2020

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Dec 26, 2019

References

cf #7299

This is a continuation of the work from #7299, in which the JLIcon class was added as the canonical way to consume icons in JupyterLab. The goal of this PR is twofold:

  • finish migrating all .svg files from the theme-x-extension packages to ui-components
  • switch all instances of icon usage in the code base over to JLIcon

Code changes

see above

User-facing changes

None, but this PR will ensure that JLIcon is fully functional for extension devs

Backwards-incompatible changes

The only thing that should break as a result of this PR are icons supplied by extensions via the now-deprecated IconRegistry and the associated API. So far as I know, adoption of IconRegistry outside of core is pretty low, so this shouldn't add too much pain to 1.0 => 2.0 migration

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Dec 26, 2019

Thanks for making a pull request to JupyterLab!

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 30, 2019

@telamonian - what is the progress on this?

@jasongrout jasongrout added this to the 2.0 milestone Dec 30, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Dec 30, 2019

@jasongrout I'm working on this right now. Before #7700 (this PR) gets pulled in, here's what needs to happen first:

AFAIK, all of the hard parts of #7700 have been finished, so the remainder is a bunch more icon migration to ui-components and JLIcon. It shouldn't take too long.

There will still need to be one last followup icon PR before 2.0 is released. That will be the PR in which:

  • IconRegistry is removed
  • All of the code related to IconRegistry is removed (possibly including the ui-components-extension package. I need to think about whether there is still any use for it for eg user overrides of icons)

We'll also need to decide if we still want to support icons-as-css-background-images in extensions. I'm leaning towards "yes", at least for 2.0, since otherwise this will become a major pain point for extension upgrades.

@jasongrout jasongrout mentioned this pull request Dec 30, 2019
25 tasks
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 30, 2019

Thanks for the update. We had originally decided on an API freeze with last Monday's beta. I've opened up discussion about extending that api freeze deadline over at #7230 (comment)

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Dec 30, 2019

Couple of questions:

  1. I notice we don't use JLIcon.get anywhere in the code base. Do you have a use case in mind for it? It seems like if drop it we might also be able to drop the name property to icons (if we also get read of the custom displayName on the icon component, which I would be in favor of).
  2. How does these icons relate to dark/light theming? Do we use a different icon system for the built in theme-able icons?
  3. Do you think you could add some docs in this PR to the common extension point docs? https://jupyterlab.readthedocs.io/en/stable/developer/extension_points.html That would also help answer my previous question, basically of the scope of this work.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 31, 2019

@telamonian and @saulshanabrook - how likely do you think it is that this will be ready to merge by the end of the week (presuming we have a lumino release in the next few days)?

@telamonian telamonian closed this Dec 31, 2019
@telamonian telamonian reopened this Dec 31, 2019
@echarles
Copy link
Contributor

@echarles echarles commented Dec 31, 2019

Linking #7711 to this PR. Prolly this PR does not fix html5 icon display, but worth to have a quick look at it before the freeze.

@echarles
Copy link
Contributor

@echarles echarles commented Dec 31, 2019

@telamonian I have opened JupyterLab Icons are not displayed in the storybook is an a repository that uses the ui-components as dependency.

Maybe you can go through this issue to confirm my analysis (loading jlab icons can fail is Browser URL is not the root one)?

A as second step, I wonder if this is easily fixed with a webpackconfig setting or if this should be handled in the ui-component package and if it is still time to onboard a fix for this in this PR?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 1, 2020

@telamonian and @saulshanabrook - how likely do you think it is that this will be ready to merge by the end of the week (presuming we have a lumino release in the next few days)?

@jasongrout It's pretty likely. All icon svgs have now been moved out of the themes and into the ui-components package, and all uses of IconRegistry have been migrated to JLIcon, which were the main goals of this PR.

What's left:

  • There's still a bunch of cases across the code base in which we use the icons-as-css-background-images approach. These need to be migrated to JLIcon
  • Some unittest fixes

Worst case scenario, some of the icons-as-css-background-images => JLIcon migration work can be punted to the future.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 1, 2020

@echarles I figured out the issue you were having in datalayer/jupyterlab-react-widgets#1, and there's now a PR with a fix over at that repo

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 1, 2020

@saulshanabrook

Thanks for reviewing this.

  1. I notice we don't use JLIcon.get anywhere in the code base. Do you have a use case in mind for it?

Look again! JLIcon.getReact and JLIcon.getElement are used in a bunch of places in the code base. Fundamentally, they're there to enable dynamic lookup of icons from strings. Some situations in which dynamic lookup is currently necessary can maybe one day be factored out. However there are some cases, such as the icons described in .json schema files, where dynamic lookup is inherently required.

It seems like if drop it we might also be able to drop the name property to icons...

Aside from dynamic lookup, name is still a good idea. At a minimum, it makes debug way easier. I'm also playing around with a bunch of ways that extension devs could make use of name to override the properties of specific icons in their own CSS.

...(if we also get read of the custom displayName on the icon component, which I would be in favor of)

Why are custom displayNames an issue?

  1. How does these icons relate to dark/light theming? Do we use a different icon system for the built in theme-able icons?

Same icon system is used for themeable icons. You just have to annotate the raw svg with the right class-es. Look at any of the svgs in ui-components/style for examples (there are still a few I haven't got around to annotating yet).

  1. Do you think you could add some docs in this PR to the common extension point docs? https://jupyterlab.readthedocs.io/en/stable/developer/extension_points.html That would also help answer my previous question, basically of the scope of this work.

The scope is that JLIcon is meant to replace all other ways of using svg icons in Jupyterlab, core and extensions. Once this PR gets pulled in, I plan to migrate all of the icons in jupyterlab-git over to JLIcon. After I've worked through that I'll write the extension dev docs

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jan 1, 2020

Thanks @telamonian for the explanations!

Some situations in which dynamic lookup is currently necessary can maybe one day be factored out. However there are some cases, such as the icons described in .json schema files, where dynamic lookup is inherently required.

Cool that makes sense.

Why are custom displayNames an issue?

What do you think about separating out the component into its own function, where the name gets passed in as a prop? Then you can see it for debugging issue and conceptually it feels a bit easier to reason about a regular defined react component than one defined anonymously in a function? Like this example from the Forwarding Ref docs?

Once this PR gets pulled in, I plan to migrate all of the icons in jupyterlab-git over to JLIcon. After I've worked through that I'll write the extension dev docs

Cool, sounds like a plan.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 2, 2020

What do you think about separating out the component into its own function, where the name gets passed in as a prop? Then you can see it for debugging issue and conceptually it feels a bit easier to reason about a regular defined react component than one defined anonymously in a function? Like this example from the Forwarding Ref docs?

@saulshanabrook Not quite sure what you mean. Also, did you forget a link somewhere?

@echarles
Copy link
Contributor

@echarles echarles commented Jan 2, 2020

@telamonian I just merged your PR https://github.com/datalayer/jupyterlab-react-widgets/issues/2 that fixes the issue with the SVG loader. I tried a few combination but apparently not the correct one. Thx a bunch for this!

Regarding this PR, my feedback is about the size of the icons which are not predictable. I would love having all icons to be rendered with a default size of e.g. 100px x 100px, with props to set the width and/or height or resizeRatio. The component would be responsible to generate the correct enclosing div to display the requested size. This would need to manually pass across all SVG files with a SVG editor like e.g. Inkscape.

I guess this can be done after if there is interest for this as this would not break the API (just adding some props).

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 3, 2020

I think this is ready to merge pending a rebase.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 3, 2020

Alllmost done. What's left:

  • There's exactly 4 more icons to fix up (out of like, 70 originally)
  • a little bit of dead code cleanup
  • need to bump the lumino deps

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 3, 2020

I'm seeing some issues locally (rotated side bar icons, missing icons and labels):

image

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 3, 2020

The above was Firefox, here's what I'm seeing in Chrome:

image

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 3, 2020

@blink1073 I haven't yet updated the lumino deps (I'm yarn link-ed in the latest Lumino master locally). I can tackle that next. I'm fairly sure that will clear up the problem you're seeing (basically, className isn't getting passed on correctly to Lumino icon nodes)

<svg
{...getReactAttrs(svgElement)}
dangerouslySetInnerHTML={{ __html: svgElement.innerHTML }}
ref={ref}
Copy link
Member

@blink1073 blink1073 Jan 6, 2020

Choose a reason for hiding this comment

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

Adding data-icon={this.name} fixes the notebook tests for me locally. I think we should keep this data attribute.

Copy link
Member Author

@telamonian telamonian Jan 6, 2020

Choose a reason for hiding this comment

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

I've gone back and forth on this. Currently, data-icon has been dumped in favor of data-iconid (see the JLIcon.resolveSvg method), but since data-iconid is a uuid, you can't use it for querying/pulling out the icon node the same way you could use data-icon. I suppose for now there can just be both data-icon and data-iconid and we can sort out which should remain later.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 6, 2020

I just pushed some changes that allow the tests to pass locally for me.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 6, 2020

fixes look good. I merged them in with my own, and hopefully now everything should be good to go

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 6, 2020

aaand now I'm seeing a bunch of (probably unrelated) test failures that I think were somehow caused by the fact that nbformat just upgraded to 5.0.0 within the last hour

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 6, 2020

I pushed a change to pin the version until jupyter/nbformat#155 is resolved. If we merge this before that gets released, I'll add an issue tracking it.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Jan 6, 2020

Thanks Steve, for coming up with the pin. All of the tests now succeed except for Windows JS. It looks like nbformat needs to be pinned separately for the Windows CI. I've taken care of it

Should we just add the pin to setup.py for now?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 6, 2020

I'm happy to leave it as a CI fix. They're working on making a new release now.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jan 6, 2020

Since we had to restart the Windows build anyway, I just removed the pins to use the new nbformat release.

@blink1073 blink1073 merged commit c5a560e into jupyterlab:master Jan 7, 2020
8 of 10 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 7, 2020

Thanks to everyone involved with this PR!

@lock lock bot added the status:resolved-locked label Feb 6, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:application pkg:apputils status:resolved-locked tag:Examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants