added notes about JLIcon's design to README #7782
Conversation
|
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
|
This should also fix #7142? |
| - every icon can create a DOM node | ||
| - every icon can create a React component | ||
| - dynamic lookup (for when absolutely needed) | ||
| - Use dynamic lookup for the few cases in which an icon can only be specified as a string (such as in json schema files) |
There was a problem hiding this comment.
So the string ids need to be unique? So we probably want to encourage namespacing the strings with the package name, like 'ui-components:myicon'?
There was a problem hiding this comment.
@jasongrout Though I don't love the idea of making the naming convention any more complex/verbose (eg under the current convention given an svg file my-icon.svg the corresponding icon name would just be my), I agree that your namespacing suggestion is the best practice going forward.
I see two issues (all dependent on the current implementation of JLIcon):
-
Purely to provide a shim for backwards compatibility, the names
foo-barandjp-fooBarIconare considered equivalent. This is due to the prevalence ofjp-fooBarIconstyle icon class names in current codebase (both core and extensions). How to implement both this shim and namespacing? -
Until you brought this up, I hadn't fully thought through what happens in the case where more than one JLIcon instances are defined using the same name. Currently, this is undefined behavior. What should happen? Two possibilities:
a. no exception, conflicts are resolved by using the most recent definition (currently, this would be hard to implement. Would need to first fully implement render-on-replace-svg behavior, or you might end up rendering a different icon for each definition).
b. exception on redefinition (maybe just a warning), only ever use the first definition of an icon.
I am open to suggestions on how to deal with the above.
|
@jtpio These particular docs probably aren't going to be of much interest/use to extension devs (mostly just notes on the conceptual design of JLIcon). I am also working on some dev docs, though |
Co-Authored-By: Jeremy Tuloup <jeremy.tuloup@gmail.com>
| - need to deprecate, then later remove, `iconClass` from a number of interfaces | ||
| - replacable | ||
| - all icons can be customized by replacing their svg dynamically during runtime | ||
| - currently, I'm leaning towards the idea that icon replacements should be an (optional) part of a theme |
There was a problem hiding this comment.
That sounds good! Previously, some icons used to be in the themes right? https://github.com/jupyterlab/jupyterlab/blob/master/packages/theme-light-extension/style/urls.css#L6
|
Thanks for these notes! This all looks good to me! |
|
This work was merged into #7767 |
lock
bot
added
the
status:resolved-locked
References
#7299, #7700, #7767
There's been a lot of recent discussion about JLIcon. In order to help clarify things, I've added some notes about the design of JLIcon to the README of ui-components.
Code changes
None
User-facing changes
None
Backwards-incompatible changes
None