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

Clean up handling of Icons under unified LabIcon #7767

Merged

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Jan 10, 2020

References

Fixes #7765

As part of #7700, I added a @jupyterlab/ui-components dependency to @jupyterlab/rendermime-interfaces. This was done in order to use JLIcon as the type of IFileType.iconRenderer. However, that brought in a number of unwanted sub-dependencies to rendermime-interfaces, such as React. This PR restores the isolation of rendermime-interfaces by:

  • Adding a minimal IJLIcon interface definition
  • Hard copying the IJLIcon definition over to rendermime-interfaces (as was done with IFileType, etc)

Code changes

In JLIcon, I removed the class method and refactored recycle into a static method called remove. Both methods were effectively already static methods, and class was redundant with the iconStyle function. These tweaks are needed for this PR, since they also fix some issues due to it no longer being safe to assume that IFileType.iconRenderer has the full functionality of JLIcon.

User-facing changes

None

Backwards-incompatible changes

The changes to the JLIcon methods are backwards incompatible. Since they're fixes, and since we're still in beta, I think they should go in

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

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

@@ -227,6 +228,20 @@ export namespace IRenderMime {
readonly default: IExtension | ReadonlyArray<IExtension>;
}

/**
* The IJLIcon interface, which supplies element, render,
* and unrender functions
Copy link
Member

Choose a reason for hiding this comment

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

I think we should mention here that there is an implementation and some example renderers in @jupyterlab/ui-components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I was hoping to see an example of how it is used.

@jasongrout
Copy link
Contributor

Would it be possible to not have a dependency on a virtual dom element? Can we abstract out that detail, and make it so they can just supply the svg icon?

@telamonian
Copy link
Member Author

Can we abstract out that detail, and make it so they can just supply the svg icon?

It depends on what exactly you mean by "the svg icon". If you mean "a string with the svg src", they can already more-or-less do that:

iconRenderer: new JLIcon({ name, svgstr });

We could also add some sugar to also allow for:

iconRenderer: { name, svgstr };

but I can forsee some problems that may cause. I'd prefer to encourage users to pre-define their icons (as JLIcon instances) and then pass one of those to iconRenderer.

Is that along the lines of what you thinking, or did you mean something else?

@jasongrout
Copy link
Contributor

jasongrout commented Jan 11, 2020

Is that along the lines of what you thinking, or did you mean something else?

What I meant was that the concept of a virtual dom is quite advanced for many users, and it looks like we are forcing them to think about virtual dom elements in order to have icons. Can we make the interface simpler by just supporting something like {name, svgstr}? If they have to create a JLIcon to easily use this icon functionality, then that means their plugin now depends on ui-components, which is essentially the same as rendermime-interfaces depending on ui-components, so we haven't gained anything by removing the dependency, practically speaking.

Just curious, what is the benefit of using a JLIcon over { name, svgstr };?

@jasongrout
Copy link
Contributor

I looked at the ui-components package a bit more. To rephrase my question/concerns above: what would be the disadvantage of deleting the @lumino/virtualdom dependency, and making rendermime-interface icon just accept a {name, svgstr} pair? The advantage is that it is very simple to understand (no knowledge of virtual dom needed) and we cut out one more dependency (and apparently we eliminate depending on ui-components for many people too, which is an even bigger issue).

@jasongrout
Copy link
Contributor

jasongrout commented Jan 11, 2020

In fact, I'm not even sure the name field is needed if we aren't reusing these icons. How about we just have:

{
    /**
     * The icon class name for the file type.
     */
    readonly iconClass?: string;

    /**
     * The icon label for the file type.
     */
    readonly iconLabel?: string;

    /**
     * The svg string for the icon.
     */
    readonly icon?: string;
}

@telamonian
Copy link
Member Author

then that means their plugin now depends on ui-components, which is essentially the same as rendermime-interfaces depending on ui-components

I think it would help the discussion if I understood the use case you have in mind a bit better. Can you point me to some examples of plugins that do depend on rendermime-interfaces but don't depend on eg application (which in turn depends on ui-components)?

Also, I was wondering: given the desired level of isolation of rendermime-interfaces w.r.t. the rest of the @jupyterlab monorepo, why is it not in @lumino instead? Though this may be a discussion for another issue.

@telamonian
Copy link
Member Author

What I meant was that the concept of a virtual dom is quite advanced for many users, and it looks like we are forcing them to think about virtual dom elements in order to have icons.

IRenderer happens to live in the virtualdom package, but it doesn't really have anything to do directly with virtual DOM:

export type IRenderer = {
    render: (host: HTMLElement) => void,
    unrender: (host: HTMLElement) => void
  };

No advanced knowledge is required, and this way users can easily set up svgs using the old as-css-background pattern, if they so desire (though I can see an argument for making unrender optional):

const fooIcon = {
    render: (host: HTMLElement) => host.className = 'my-foo-icon',
    unrender: (host: HTMLElement) => void
  };

Really though, I'm hoping to encourage that most people just use JLIcon. On that track, from your perspective would it help if JLIcon were split up so as to have a non-react using base/sibling class? Because then JLIcon could be used in rendermime-interfaces without adding any major deps.

@jasongrout
Copy link
Contributor

I think it would help the discussion if I understood the use case you have in mind a bit better. Can you point me to some examples of plugins that do depend on rendermime-interfaces but don't depend on eg application (which in turn depends on ui-components)?

All of the renderers in https://github.com/jupyterlab/jupyter-renderers/tree/master/packages except for the mathjax and katex renderer. The pdf renderer in core. I think the main intended usecase for rendermime-interface was renderers that don't depend on anything else from JupyterLab.

Also, I was wondering: given the desired level of isolation of rendermime-interfaces w.r.t. the rest of the @jupyterlab monorepo, why is it not in @lumino instead? Though this may be a discussion for another issue.

We could possibly do that, but it seems to fit more naturally into JLab, since it's a very restricted stable interface for a JLab plugin for rendering mimetypes in JLab. I mean, it is pretty specific with the notion of filetypes, etc., which fit more with JLab than more general lumino, I think.

@jasongrout
Copy link
Contributor

Really though, I'm hoping to encourage that most people just use JLIcon.

The easiest way to do that, I think, is to just make the icon an svg string, and we wrap it in a JLIcon.

As for using css - we already have an iconClass field too - they could just use that, right?

Again, what would be the disadvantage of just having icon be an svg string (and we create the JLIcon when we create the rendermime plugin from their extension)? I don't see a disadvantage, so I'm not sure what we would be trading off doing this.

@jasongrout
Copy link
Contributor

We could also add some sugar to also allow for:

iconRenderer: { name, svgstr };

but I can forsee some problems that may cause.

What problems did you foresee?

@jasongrout jasongrout added this to the 2.0 milestone Jan 14, 2020
@telamonian
Copy link
Member Author

telamonian commented Jan 14, 2020

@jasongrout I'm prepping an answer for you. Part of the reason why it's tricky to think through is that I haven't written anything down about the design of JLIcon in months (partly because it's been in flux until recently). To that end I'm also adding some some docs about JLIcon (for now, I'll stick them in the ui-components README).

Would you happen to have some time to talk about icons tomorrow? I think it would be good for us to sync up about this, and it'll probably be difficult for me to explain all of it during the weekly meeting.

@jasongrout
Copy link
Contributor

Thanks! I can't talk tomorrow (and won't make the meeting tomorrow), but could talk on Thursday.

@telamonian
Copy link
Member Author

As for using css - we already have an iconClass field too - they could just use that, right?

I plan to deprecate the iconClass field in IFileType and all similar interfaces, with the goal of removing it in jlab 3.0 (I kind of want to see how the JLIcon rollout goes first, though). The current uses of iconClass (to set a background image and/or styling for said image via css class) are redundant w.r.t. the functionality of JLIcon. Getting rid of iconClass reduces the possibility of weird styling clashes, would simplify some implementation issues related to dynamic icon lookup, and would be one more step towards the goal of having exactly one simple (as possible) way to set up and use icons in jlab.

Originally, I was just going to remove iconClass as part of implementing JLIcon. I hesitated when I realized that doing so would make the jlab 2.0 migration process much rockier for many existing jlab extensions (ie any extension that passes an icon into core, so anything with a filetype, settings, sidebar, toolbar button, etc icon). In the end, I implemented a slightly refined version of the backwards compatibility code brought up in #7685:

  • For an icon named foo, the icon lookup code will match to both foo and jp-FooIcon
  • When iconRenderer is not specified, dynamic lookup is performed by looping through all class names in iconClass and returning the first match
  • If there are no matches, create a blank element with class iconClass (ie use the old icon-as-background-image behavior)

@jasongrout
Copy link
Contributor

Thanks, and thanks for writing things up in #7782. I'm convinced even more that the rendermime interfaces should not have any notion of jlicon after reading through the above comments and your very helpful writeup:

  1. The JLIcon interface is still iterating and likely to change. If a rendermime interface needs to create a JLIcon, it will need to be updated when there are changes to how JLIcons work.
  2. It's not likely that a rendermime extension will need to define an icon that someone else in the system will need to use independently
  3. Anyways, a major point here is that the rendermime interface is very simple and constrained. An svg string fills that goal.

I'm still curious: What exactly is the benefit of a rendermime extension creating a JLIcon, as opposed to giving an svg string and us creating the JLIcon for it?

@telamonian
Copy link
Member Author

@blink1073 I've updated the docs. There's now info in the extension point docs, and a note/link in the extension migrations docs.

@blink1073 blink1073 changed the title Remove ui-components dep from rendermime interfaces Clean up handling of Icons under unified LabIcon Jan 30, 2020
Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks! I fixed the docs build and added a changelog entry. I'll get this merged and released tomorrow morning.

@telamonian
Copy link
Member Author

I fixed the docs

Ah. I could not for the life of me figure out the right type for typescript + react code in rst. Thanks Steve!

@blink1073
Copy link
Member

@telamonian, are you looking into the virtualdom signature issue mentioned on gitter?

@telamonian
Copy link
Member Author

@blink1073 I am, and I'm working on it right now

@telamonian
Copy link
Member Author

@blink1073 I have it worked out. I'll try to upload the fix early tomorrow

@blink1073
Copy link
Member

👍

@blink1073
Copy link
Member

@telamonian, were there no changes required in lumino?

@telamonian
Copy link
Member Author

telamonian commented Feb 3, 2020

were there no changes required in lumino?

@blink1073 Nope, no changes to Lumino were needed. If we pull this PR in and then do a new release that will fix all of the typing issues reported on gitter (though extension devs will need to update their package.json to point to said new release).

There's a separate issue as to whether the recent changes to @lumino/virtualdom (in jupyterlab/lumino#44) were "truly" backwards compatible. In my view, the answer is yes, and the real problem stems from an unsafe "widening" of the IRenderer.render typing made on the Jupyterlab side in #7700. The good news there is that the changes from #7700 were only ever included in the beta releases of jlab 2.0.

Thus, I think the way forward is to pull in this PR and do a release. I'll add an issue with the full details of the typing snafu, and we can debate whether to rollback Lumino there (but like I said, I don't think we need to).

@blink1073
Copy link
Member

Sounds good, onward!

@blink1073 blink1073 merged commit 0c609d6 into jupyterlab:master Feb 3, 2020
telamonian added a commit to telamonian/jupyterlab that referenced this pull request Feb 4, 2020
blink1073 pushed a commit that referenced this pull request Feb 4, 2020
followup #7767: small bugfix to lookup in UNSTABLE_getReact
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Mar 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.