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

IRenderer cleanup; normalize icon fields across all interfaces #46

Merged
merged 13 commits into from Feb 10, 2020

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Feb 9, 2020

needed for jupyterlab/jupyterlab#7864

This PR has two goals:

  • I recently got this bit of feedback from one of the LabIcon early adopters (@vidartf via gitter):

    by inspecting the lab source, I figured iconRenderer was the attribute to set

    He was talking about figuring out that Title.iconRenderer was the field you're supposed to pass a LabIcon object into. Which, fair point, it's pretty confusing that LabIcon gets passed into the Foo.icon field for all interfaces except those defined in Lumino. On top of that, iconRenderer as a name is kind of clunky and maybe a bit conceptually confusing. Given all that, I want to experiment with reclaiming the deprecated Title.icon field, and see if I can do so in a backwards compatible way

  • Given that it's possible to reclaim Title.icon, I want to normalize the behavior of the .icon field in all of the relevant interfaces across Lumino, so that all icons in Lumino are able to use custom renderers. Once that's done, I'll be able to finally migrate the last few holdout icons in Jupyterlab to LabIcon and resolve the remaining issues with that (see Post LabIcon cleanup; migrate last holdouts to LabIcon; fix icon styling jupyterlab#7864)

There's also a minor fix to the custom renderer implementation in virtualdom that needs getting in (the attrs and children args currently aren't getting passed in to a custom renderer in a consistent fashion)

@telamonian telamonian marked this pull request as ready for review February 10, 2020 11:32
@telamonian
Copy link
Member Author

telamonian commented Feb 10, 2020

I've now implemented everything I described in the top post, and added a bunch of extra tests to cover all of the new .icon backwards compatibility stuff. This PR is ready for review

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, thanks!

@blink1073 blink1073 merged commit bf35b5f into jupyterlab:master Feb 10, 2020
@blink1073
Copy link
Member

Released:

  • @lumino/commands@1.10.0
  • @lumino/virtualdom@1.6.0
  • @lumino/widgets@1.11.0

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants