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
Jlicon migration #7700
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
@telamonian - what is the progress on this? |
@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 There will still need to be one last followup icon PR before 2.0 is released. That will be the PR in which:
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. |
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) |
Couple of questions:
|
af7dadc
to
19e34ce
Compare
@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)? |
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. |
@telamonian I have opened JupyterLab Icons are not displayed in the storybook is an a repository that uses the 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? |
@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 What's left:
Worst case scenario, some of the |
@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 |
Thanks for reviewing this.
Look again!
Aside from dynamic lookup,
Why are custom
Same icon system is used for themeable icons. You just have to annotate the raw svg with the right
The scope is that |
Thanks @telamonian for the explanations!
Cool that makes sense.
What do you think about separating out the component into its own function, where the
Cool, sounds like a plan. |
@saulshanabrook Not quite sure what you mean. Also, did you forget a link somewhere? |
@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 I guess this can be done after if there is interest for this as this would not break the API (just adding some props). |
I think this is ready to merge pending a rebase. |
Alllmost done. What's left:
|
5b743d6
to
d8c0968
Compare
@blink1073 I haven't yet updated the lumino deps (I'm |
<svg | ||
{...getReactAttrs(svgElement)} | ||
dangerouslySetInnerHTML={{ __html: svgElement.innerHTML }} | ||
ref={ref} |
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.
Adding data-icon={this.name}
fixes the notebook tests for me locally. I think we should keep this data attribute.
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'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.
I just pushed some changes that allow the tests to pass locally for me. |
fixes look good. I merged them in with my own, and hopefully now everything should be good to go |
aaand now I'm seeing a bunch of (probably unrelated) test failures that I think were somehow caused by the fact that |
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. |
Thanks Steve, for coming up with the pin. All of the tests now succeed except for Windows JS. It looks like Should we just add the pin to |
I'm happy to leave it as a CI fix. They're working on making a new release now. |
Since we had to restart the Windows build anyway, I just removed the pins to use the new |
Thanks to everyone involved with this PR! |
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:.svg
files from thetheme-x-extension
packages toui-components
JLIcon
Code changes
see above
User-facing changes
None, but this PR will ensure that
JLIcon
is fully functional for extension devsBackwards-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 ofIconRegistry
outside of core is pretty low, so this shouldn't add too much pain to 1.0 => 2.0 migration