{{ message }}
/ jupyterlab Public

Merged
Merged

Clean up handling of Icons under unified LabIcon#7767

Conversation

This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters

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.

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 bot commented Jan 10, 2020

 Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link:

reviewed
Show resolved Hide resolved
reviewed
 @@ -227,6 +228,20 @@ export namespace IRenderMime { readonly default: IExtension | ReadonlyArray; } /** * The IJLIcon interface, which supplies element, render, * and unrender functions

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

jasongrout Jan 10, 2020

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

jasongrout commented Jan 10, 2020

 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 commented Jan 10, 2020

 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?

reviewed
Show resolved Hide resolved

jasongrout commented Jan 11, 2020 • edited

 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 commented Jan 11, 2020

 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 commented Jan 11, 2020 • edited

 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 commented Jan 11, 2020

 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 commented Jan 11, 2020

 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 commented Jan 12, 2020

 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 commented Jan 12, 2020

 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 commented Jan 12, 2020

 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?

added this to the 2.0 milestone Jan 14, 2020

telamonian commented Jan 14, 2020 • edited

 @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 commented Jan 14, 2020

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

telamonian commented Jan 14, 2020

 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)

mentioned this pull request Jan 14, 2020

jasongrout commented Jan 14, 2020

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

added 3 commits Jan 18, 2020
 replaced JLIcon (and unwanted React sub-dep) in rendermime-interfaces 
 40d8bc6 
 improved accuracy of IFileType.iconRenderer typing 
 542a428 
 use render method for listings icons, simplify iconRenderer typing 
 00adfd5 
 added note/link about LabIcon to extension migration docs 
 6ec8cf3 

telamonian commented Jan 29, 2020

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

changed the title Remove ui-components dep from rendermime interfaces Clean up handling of Icons under unified LabIcon Jan 30, 2020
 fix docs and add changelog entry 
 b03c8d3 
approved these changes

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

telamonian commented Jan 30, 2020

 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!

 update ci_script 
 19e94a6 

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

telamonian commented Jan 31, 2020

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

telamonian commented Feb 1, 2020

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

 👍

 fixed typing error wrt latest lumino; removed unsafe widening of IRen… 
 947f932 
…derer
 allow other svg attributes in test 
 282584c 

 @telamonian, were there no changes required in lumino?

telamonian commented Feb 3, 2020 • edited

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

 Sounds good, onward!

merged commit 0c609d6 into jupyterlab:master Feb 3, 2020
8 of 10 checks passed
mentioned this pull request Feb 3, 2020
added a commit to telamonian/jupyterlab that referenced this issue Feb 4, 2020
 followup jupyterlab#7767: small bugfix to lookup in UNSTABLE_getReact 
 687c259 
mentioned this pull request Feb 4, 2020
pushed a commit that referenced this issue Feb 4, 2020
 Merge pull request #7846 from telamonian/fix-launcher-icon-lookup 
 ff5db65 
followup #7767: small bugfix to lookup in UNSTABLE_getReact
mentioned this pull request Feb 5, 2020