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.

Sign up for GitHub

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

Jump to bottom

added notes about JLIcon's design to README #7782

Closed
wants to merge 3 commits into from
Closed

added notes about JLIcon's design to README #7782

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5a7737
added notes about JLIcon's design to README
telamonian Jan 14, 2020
fcd16f5
fixed typo
telamonian Jan 14, 2020
a297d5d
fixed typo
telamonian Jan 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 44 additions & 0 deletions packages/ui-components/README.md
View file
Open in desktop
Expand Up @@ -43,3 +43,47 @@ The icons are organized into various categories in `./style/icons`, based on whe
- launcher
- statusbar
- settingeditor

## Notes on design of JLIcon

Design goals for JLIcon (already implemented):

- the one true icon
- create a single, canonical, simple (as possible) way to set up and use icons in jlab
- every icon is a symbol
- each icon is defined exactly once, and is thereafter referred to via a unique symbol
- to use an icon `fooIcon` outside of the file in which it is defined, the pattern will be to import `fooIcon`
- this enables compile-time checking; helps to ensure that icons are specified correctly when used (as opposed to the old pattern of specifying icon via a `string` with name or className)
- every icon is flexible
- can used in any context in which icons are used in jlab
- every icon can be passsed into Lumino
telamonian marked this conversation as resolved.
Show resolved Hide resolved
- 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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'?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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):

  1. Purely to provide a shim for backwards compatibility, the names foo-bar and jp-fooBarIcon are considered equivalent. This is due to the prevalence of jp-fooBarIcon style icon class names in current codebase (both core and extensions). How to implement both this shim and namespacing?

  2. 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.

- In all other cases, force (or at least strongly encourage) the pattern of import-icon
- reusable
- every defined icon can be used any number of times in any set of contexts
- where an icon is defined should not matter; all icons defined in core and extensions are reusable in any other extension

Design goals for JLIcon (partially implemented):

- remove all other ways of creating icons (though leave an escape hatch)
- 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
Copy link
Member

Choose a reason for hiding this comment

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

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

- ideally, replacability will not depend on dynamic lookup
- whenever its svg is replaced, all visible copies of an icon should immediately rerender
- possible implementations:
- signals
- observables
- `MutationObserver`

Possible design patterns for JLIcon:

1. each icon is a `class`. The icon is used by creating a new instance
2. each icon is a function with methods (ie a callable instance). The icon is used by calling the appropriate method
3. each icon is an instance of a well-defined `class`. The icon is used by calling the appropriate instance method

Patterns 1) and 2) were both initially investigated (see [jupyterlab/jupyterlab#7299](https://github.com/jupyterlab/jupyterlab/pull/7299)). Pattern 3) was found to be easiest to reason about, and had a desirable set of tradeoffs relating to features like dynamic lookup and replacability (eg you can replace the svg of an icon by just setting the `svgstr` field of the icon instance).