-
Notifications
You must be signed in to change notification settings - Fork 107
Add lucide v12 #2250
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 lucide v12 #2250
Conversation
|
9f618d2 to
f083632
Compare
|
|
||
| const CONFLICTING_NAMES = ['Infinity', 'Map'] | ||
|
|
||
| function generateIndex() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to update to a new Lucid version, this function has to be run to generate an updated list of icons in the index.tsx. I will add this info to the documentation (in another PR).
| @@ -0,0 +1,114 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example mapping file between old and new icons. If Alex is ready with the mapping, this is going to be changed for that version.
f083632 to
4877ec6
Compare
| @@ -0,0 +1,49 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed because if you use the latest version of React-window it supplies its own types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not needed
packages/__docs__/package.json
Outdated
| "moment": "^2.30.1", | ||
| "react": "18.3.1", | ||
| "react-dom": "18.3.1", | ||
| "react-window": "^1.8.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the latest version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still using an old version
matyasf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, overall I think it looks good :)
Can you also please write a list to the upgrade guide about which props are removed/changed compared to old icons?
| const accessibilityProps: Record<string, string | boolean> = {} | ||
| if (title) { | ||
| accessibilityProps['aria-label'] = description | ||
| ? `${title}: ${description}` | ||
| : title | ||
| accessibilityProps['role'] = 'img' | ||
| } else { | ||
| accessibilityProps['aria-hidden'] = 'true' | ||
| accessibilityProps['role'] = 'presentation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see in the code the old InlineSVG added a <title> tag when the title prop was specified; description added a <description> that and connected these to the svg with aria-labelledby. Why is this here differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed that title stays, description will be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTML props are spread over to the underlying <svg> -> add this to ugprade guide.
f34af3a to
affcac0
Compare
matyasf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comments
| color?: string | ||
| rotate?: '0' | '90' | '180' | '270' | ||
| rotate?: LucideIconWrapperProps['rotate'] | ||
| bidirectional?: boolean | ||
| inline?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
themse need the same prop ref too, so if we change the prop, this one changes too
| @@ -0,0 +1,49 @@ | |||
| /* | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not needed
packages/__docs__/package.json
Outdated
| "moment": "^2.30.1", | ||
| "react": "18.3.1", | ||
| "react-dom": "18.3.1", | ||
| "react-window": "^1.8.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is still using an old version
affcac0 to
b8a0fca
Compare
|
|
||
| // Empty object constant for cellProps to maintain referential equality | ||
| // and prevent unnecessary re-renders of all cells | ||
| const EMPTY_CELL_PROPS = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an empty object without a reference is assigned to cellProps on Grid it keeps rerendering, this trick prevents it.
|
|
||
| // Empty object constant for cellProps to maintain referential equality | ||
| // and prevent unnecessary re-renders of all cells | ||
| const EMPTY_CELL_PROPS = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an empty object without a reference is assigned to cellProps on Grid it keeps rerendering, this trick prevents it.
matyasf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good now
b8a0fca to
0913b71
Compare
feat(ui-icons-lucide): Add Lucide Icons Package
Overview
This commit introduces a new
@instructure/ui-icons-lucidepackage, providing 1,500+ modern icons from the Lucide icon library with full InstUI theming integration. This replaces the deprecated legacy icon system with a more flexible, maintainable solution built on a pure API.What's Added
New Package:
ui-icons-lucidewrapLucideIconHOC for custom icon wrapping with InstUI featuresmapping.json) for migrating from legacy InstUI icons to LucideKey Features
Dual API Support
size="lg",color="successColor"size={24},color="#ff0000"InstUI Theming Integration
xs,sm,md,lg,xl,2xlRTL Support
bidirectionalprop (default:true)Rotation & Positioning
Accessibility
titleanddescriptionpropsrole="img"andaria-labelwhen title is providedaria-hidden="true"for decorative iconsDocumentation Gallery Updates
react-windowfor optimal performance with 1,500+ iconsMigration Support
mapping.jsonprovides direct mappings from legacy InstUI icons to Lucide equivalentsTechnical Implementation
Architecture
wrapLucideIcon()HOCuseTheme()to access InstUI theme tokenselementRefpattern for DOM accessBuild System
generateIndex.tsscript auto-generates exports for all 1,500+ iconsTesting Plan
Manual Testing
1. Icon Gallery Functionality
2. Icon Usage Testing
Breaking Changes
None - this is an additive change. Legacy icons remain functional but deprecated.
This PR description was created with assistance from Claude AI