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

Post LabIcon cleanup; migrate last holdouts to LabIcon; fix icon styling #7864

Merged
merged 47 commits into from Feb 24, 2020

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Feb 9, 2020

References

fixes #7859, #7869, #7887

This PR has two goals:

  • Fix up remaining weird edge case uses of LabIcon and remove all LabIcon.UNSTABLE_foo methods
    • All gone!
  • migrate remaining holdout icon-as-css-background cases to LabIcon (the menu icons (see #7859) and a handful of others (like the log console icon, I think?))
    • 100% of icons in jlab core are now set up and rendered using LabIcon

Fixing the above also involves some changes to lumino, so this PR includes a bump to lumino deps.

Also, some needed fixes for the icon styling system ended up involving some minor API breaking changes (some deprecated cruft was removed from a couple of function sigs, see below). So now this PR also includes a significant cleanup of icon styling code (see ui-components/src/style/icon.ts). The big news from that is that now users will be able to specify their own custom icon styling much more freely/completely than before.

Code changes

A lot

User-facing changes

No more visible icon regressions

Backwards-incompatible changes

I deprecated/removed RendererClass from LabIcon.IOptions (which is used as the type of the args for LabIcon.constructor). RendererClass was part of a now-removed renderer mixin scheme for LabIcon, and was redundant/confusing wrt some of the other fields in LabIcon.IOptions.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Feb 9, 2020

Thanks for making a pull request to JupyterLab!

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

@telamonian telamonian changed the title Post labicon cleanup; migrate last holdouts to LabIcon (needs jupyterlab/lumino#44) Post LabIcon cleanup; migrate last holdouts to LabIcon (needs jupyterlab/lumino#44) Feb 9, 2020
@telamonian telamonian changed the title Post LabIcon cleanup; migrate last holdouts to LabIcon (needs jupyterlab/lumino#44) Post LabIcon cleanup; migrate last holdouts to LabIcon (needs jupyterlab/lumino#46) Feb 9, 2020
@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 22, 2020

Progress update:

AFAIK, all icon issues are now fixed in this PR. If you spot differently, please tell me. Two exceptions:

  • CommandPalette icons
    • fixed
  • The "clear" icon in the log console has been temp changed to just closeIcon
    • I created our own clearIcon

Getting submenus to render icons correctly took some true hackish weirdness in MenuSvg, if anyone cares to review:

insertItem(index: number, options: Menu.IItemOptions): Menu.IItem {
if (options.submenu?.renderer === Menu.defaultRenderer) {
// create a "view" of the submenu with a different default renderer
const submenu = Object.create(options.submenu, {
renderer: {
configurable: true,
enumerable: true,
value: MenuSvg.defaultRenderer
}
});
// Widget.title is an AttachedProperty, and needs special handling
submenu.title.label = options.submenu.title.label;
submenu.title.mnemonic = options.submenu.title.mnemonic;
submenu.title.icon = options.submenu.title.icon;
submenu.title.iconClass = options.submenu.title.iconClass;
submenu.title.iconLabel = options.submenu.title.iconLabel;
submenu.title.caption = options.submenu.title.caption;
submenu.title.className = options.submenu.title.className;
submenu.title.dataset = options.submenu.title.dataset;
options.submenu = submenu;
}
return super.insertItem(index, options);
}

@telamonian telamonian marked this pull request as ready for review Feb 24, 2020
@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 24, 2020

@blink1073 @jasongrout I think this PR is now ready to go in, and should fix all of the currently outstanding icon issues.

There's one small breaking change now: I removed rendererClass from the args of the LabIcon constructor (the details have been added to the first post above).

@telamonian telamonian changed the title Post LabIcon cleanup; migrate last holdouts to LabIcon (needs jupyterlab/lumino#46) Post LabIcon cleanup; migrate last holdouts to LabIcon; fix icon styling Feb 24, 2020
@github-actions github-actions bot added tag:Design System CSS tag:CSS labels Feb 24, 2020
telamonian added 2 commits Feb 24, 2020
- bug could have caused a cached stylesheet to be incorrectly reused
@telamonian
Copy link
Member Author

@telamonian telamonian commented Feb 24, 2020

I did an audit of the remaining icon-related CSS code left in the codebase. There are currently exactly 5 icons that are still being rendered using the old icon-as-css-background behavior:

  • The up/down/left/right caret icons on the buttons on the end of the scrollbars of any lumino Datagrid
  • The magnifying glass/search icon to the right of the search bar at the top of the command palette

The datagrid scrollbar icons will probably require some more fixes in lumino. In any case, migrating these particular icons to LabIcon isn't a high priority. Since they are all shown on top of a fixed color background (that doesn't change with the theme), their appearance should be correct in all themes regardless of rendering method

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 24, 2020

I did another pass of the code and tested it on binder, looks good, thanks @telamonian!

@blink1073 blink1073 merged commit 869c2bc into jupyterlab:master Feb 24, 2020
10 of 11 checks passed
@jasongrout jasongrout added this to the 2.0 milestone Feb 24, 2020
@lock lock bot added the status:resolved-locked label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants