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

Add icon component to the toolkit #123

Merged
merged 9 commits into from
Aug 5, 2024

Conversation

Mehak261124
Copy link
Contributor

@Mehak261124 Mehak261124 commented Jul 30, 2024

This pull request introduces a new Icon component in the toolkit, extending the FASTElement from Microsoft's Fluent Design System (FAST). The component is designed to be leaner and more streamlined compared to the existing LabIcon class, ensuring a clearer implementation and easier usage.

Key Features:

Inheritance from FASTElement: Leverages the extensibility of FASTElement to integrate seamlessly with the Fluent Design System.
Simplified Icon Registration: Enables registering icons dynamically without using the new syntax, enhancing developer convenience.
Direct Use in Web Applications: Icons can be incorporated using the "@" syntax, simplifying their usage across various web applications.
Non-Interactive Design: The icon does not interact with or modify its container, maintaining a clear separation of concerns.
React Integration: Includes an encapsulated React component version of the icon, maintaining consistency and usability across our React-based projects.
Technical Details:
New Folder and Files:

Created a new directory packages/components/src/icon/ containing:
index.ts: Defines the Icon class and handles icon registration.
icon.stories.ts: Provides Storybook stories to demonstrate the icon implementations and their variations.

Implementation:

Utilized @microsoft/fast-element for custom elements.
The Icon class utilizes a static map to manage icon registration and retrieval.
SVG content is dynamically loaded based on the name attribute, with a default icon fallback.

Fixes #119

Copy link

Binder 👈 Launch a Binder on branch Mehak261124/jupyter-ui-toolkit/toolkit-icon

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @Mehak261124

This looks great. I added documentation to improve the code and looking at the LabIcon code in JupyterLab I realized we need to make two changes:

  • Add a method static setDefaultIcon(svgStr: string): void to be able to customize the default icon.
  • Allow for registration of icon with an existing name. In such case, it should warn about that: console.warn(`Redefining previously loaded icon svgstr. name: ${name}, svgstrOld: ${icon.svgstr}, svgstr: ${svgstr}`);
    And it should rerender all existing icons; something like that may work:
    document.querySelectorAll(`jp-icon[name="${name}"]`).forEach(node => { node.name = "";
    node.name = name;
    });

packages/components/src/icon/index.ts Show resolved Hide resolved
packages/components/src/icon/index.ts Show resolved Hide resolved
packages/components/src/icon/index.ts Show resolved Hide resolved
packages/components/src/icon/index.ts Show resolved Hide resolved
packages/components/src/icon/index.ts Show resolved Hide resolved
@fcollonval fcollonval added the enhancement New feature or request label Jul 31, 2024
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @Mehak261124

The code looks good. I have some suggestions, mainly to test changing the icon definition.

packages/components/src/icon/index.ts Show resolved Hide resolved
packages/components/src/icon/icon.stories.ts Outdated Show resolved Hide resolved
packages/components/src/icon/icon.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @Mehak261124

The remaining failures are due to flakiness.

@fcollonval fcollonval merged commit 1583278 into jupyterlab-contrib:main Aug 5, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an icon component
2 participants