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

progress on icons: added inline svg icon support to toolbar buttons #7192

Merged
merged 14 commits into from Sep 16, 2019

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Sep 11, 2019

References

followup to #6034

commit 0d59934 included some related follow-up to #7182:
fixes: #7189
fixes: jupyterlab/jupyterlab-git#411
fixes: QuantStack/jupyterlab-drawio#45

Code changes

Toolbar buttons now support inline SVG icons. Here's how would add a new toolbar button icon to Jlab core, using a foo-bar.svg file:

  • copy foo-bar.svg to ui-components/style/icons/toolbar
  • When you create a new ToolbarButton, enter the icon name in the iconClassName parameter of the constructor. Given an icon file named foo-bar.svg, either of the following will be valid:
return new ToolbarButton({
      iconClassName: 'foo-bar',
      ...
    });

or

return new ToolbarButton({
      iconClassName: 'jp-FooBarIcon',
      ...
    });

I also improved/unified fallback to old icon handling behavior in case of missing icon.

User-facing changes

Adds (optionally themeable) inline svg icons for toolbars.

Backwards-incompatible changes

also added some docstrings to the icon-related interfaces
@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Sep 11, 2019

Thanks for making a pull request to JupyterLab!

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

@telamonian telamonian marked this pull request as ready for review Sep 11, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Sep 11, 2019

This is now ready to go

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 11, 2019

Just checking - is this (should this be) backwards compatible with 1.0?

@telamonian
Copy link
Member Author

@telamonian telamonian commented Sep 12, 2019

Just checking - is this (should this be) backwards compatible with 1.0?

It definitely should be. I am sure that this PR doesn't regress anything in terms of the fixes made in #7182

@telamonian
Copy link
Member Author

@telamonian telamonian commented Sep 14, 2019

This PR now also includes some followup to #7182 in the form of more workarounds to deal with newly discovered more and different issues arising from Jlab 1.0.x installations bringing in Jlab 1.1.x packages when rebuilding against extensions.

@telamonian telamonian added the bug label Sep 14, 2019
@telamonian
Copy link
Member Author

@telamonian telamonian commented Sep 16, 2019

@jasongrout Can you please take a look and sign off on this PR? It should fix the remaining reported UI bugs that were caused by package version mismatches.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

It's in my queue for today :).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

Depending on how we define our public API, this could be seen as a breaking change for some packages, in that it moves CSS classes and variables from one package to another. The end result is JupyterLab as a whole is not incompatible, but individual packages are, if we consider the CSS classes/variables part of a package's public API.

Still, I think fixing the breakage we are seeing in 1.0.2 is worth merging this. I think even more we need a really clear story about exactly what we are promising when we say we are backwards compatible.

@jasongrout jasongrout merged commit 1c5175f into jupyterlab:master Sep 16, 2019
9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

@blink1073 - if you have some time, I'd love to have a second review of this before backporting to 1.1.x.

@telamonian
Copy link
Member Author

@telamonian telamonian commented Sep 16, 2019

we need a really clear story about exactly what we are promising when we say we are backwards compatible.

I think that's a large part of what caused the bugs fixed in #7182 and this PR. Like you say, there seem to be two types of backwards compatibility that we're having to deal with here:

  1. Compatibility wrt Jupyterlab as a whole
  2. Compatibility wrt individual packages in @jupyterlab

When I wrote the icon handling code in #6034 in the first place, I tried to make it backwards compatible in the sense of 1. I think that part I was successful at. I wasn't even thinking about 2.

We shouldn't need to consider the backwards compatibility of a core package wrt other core packages (I suppose we're stuck doing that until 2.0.0, in any case). So I think all of the tricky cases that we need to consider are for non-Jlab applications that depend on a subset of the core packages.

For example, for the sake of non-Jlab applications should we add "resolutions" to all of the core packages to ensure against Jlab package version mismatch?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

For example, for the sake of non-Jlab applications should we add "resolutions" to all of the core packages to ensure against Jlab package version mismatch?

I think a more acceptable solution would be to change all of our inter-dependencies to ~ instead of ^. But at that point, are we really following semver anymore?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 16, 2019

(resolutions won't work because they are ignored except at the top level, IIRC. They work in JLab because we control the top-level compilation)

@vidartf
Copy link
Member

@vidartf vidartf commented Sep 17, 2019

We shouldn't need to consider the backwards compatibility of a core package wrt other core packages

You might get away with this for extensions, but please do not do this for library packages. One of the core principles for lab has been to be more modular so that others can reuse part of the code, and to do it safely by following semver.

@vidartf
Copy link
Member

@vidartf vidartf commented Sep 17, 2019

Also, there might be custom lab apps (e.g. phoila) that only use a subset of the core extensions (or replacing a core extension with a custom version), so reshuffling of features can quickly break those.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 17, 2019

LGTM. As part of this release I'd like to test against jlab 1.0 + some pinned extensions (which will be added to the release script).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 17, 2019

Also fixes #7221

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 17, 2019

@meeseeksdev backport to 1.1.x

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 18, 2019

@meeseeksdev backport to 1.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Sep 18, 2019
jasongrout added a commit that referenced this issue Sep 18, 2019
…2-on-1.x

Backport PR #7192 on branch 1.x (progress on icons: added inline svg icon support to toolbar buttons)
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 18, 2019

@meeseeksdev backport to 1.1.x

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Sep 18, 2019

Something went wrong ... Please have a look at my logs.

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Sep 18, 2019
jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Sep 18, 2019
… support to toolbar buttons

(cherry picked from commit 0a989e6)
jasongrout added a commit that referenced this issue Sep 18, 2019
Backport PR #7192: progress on icons: added inline svg icon support to toolbar buttons
@jasongrout jasongrout removed this from the 1.2 milestone Oct 11, 2019
@jasongrout jasongrout added this to the 2.0 milestone Oct 11, 2019
@lock lock bot added the status:resolved-locked label Nov 10, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug enhancement pkg:ui-components status:resolved-locked
Projects
None yet
4 participants