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
Clean up handling of Icons under unified LabIcon #7767
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
@@ -227,6 +228,20 @@ export namespace IRenderMime { | |||
readonly default: IExtension | ReadonlyArray<IExtension>; | |||
} | |||
|
|||
/** | |||
* The IJLIcon interface, which supplies element, render, | |||
* and unrender functions |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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? |
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 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 Just curious, what is the benefit of using a JLIcon over |
I looked at the ui-components package a bit more. To rephrase my question/concerns above: what would be the disadvantage of deleting the |
In fact, I'm not even sure the {
/**
* 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;
} |
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 |
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
Really though, I'm hoping to encourage that most people just use |
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.
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. |
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 |
What problems did you foresee? |
@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. |
Thanks! I can't talk tomorrow (and won't make the meeting tomorrow), but could talk on Thursday. |
I plan to deprecate the Originally, I was just going to remove
|
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:
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? |
@blink1073 I've updated the docs. There's now info in the extension point docs, and a note/link in the extension migrations docs. |
There was a problem hiding this 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.
Ah. I could not for the life of me figure out the right type for typescript + react code in rst. Thanks Steve! |
@telamonian, are you looking into the virtualdom signature issue mentioned on gitter? |
@blink1073 I am, and I'm working on it right now |
@blink1073 I have it worked out. I'll try to upload the fix early tomorrow |
👍 |
@telamonian, were there no changes required in |
@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 There's a separate issue as to whether the recent changes to 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! |
followup #7767: small bugfix to lookup in UNSTABLE_getReact
References
Fixes #7765
As part of #7700, I added a
@jupyterlab/ui-components
dependency to@jupyterlab/rendermime-interfaces
. This was done in order to useJLIcon
as the type ofIFileType.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:IJLIcon
interface definitionIJLIcon
definition over to rendermime-interfaces (as was done withIFileType
, etc)Code changes
In
JLIcon
, I removed theclass
method and refactoredrecycle
into a static method calledremove
. Both methods were effectively already static methods, andclass
was redundant with theiconStyle
function. These tweaks are needed for this PR, since they also fix some issues due to it no longer being safe to assume thatIFileType.iconRenderer
has the full functionality ofJLIcon
.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