feat(context-providers): Adding Context providers #3315
Conversation
|
I would vote against this and would add support for this use case by pointing people to use css variables. Purely due to the introduced complexity and performance impact. |
github-actions
bot
added
⚛️ react package
github-actions
bot
added
📦 dependencies
|
Docs will be written in a different branch |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
I have always been very curious why Lucide does not implement a provider. Has there been any previous discussion about this? We want to default our size to |
|
@mathhulk Not sure which framework and package you are using, but isn't that possible with CSS. See: https://lucide.dev/guide/advanced/global-styling |
|
this is the only PR on making a context to adjust default props but to also possibly pass a default class to have to create a new component or pass to every lucide icon this ? |
Co-authored-by: Copilot <[email protected]>
|
@joceqo So if i'm understand correctly. You would like the adjust classNames as well? |
|
The main thing i would want to change currently with lucide is to have it possible to set width and height to 1em by default so i could wrap icon in Typographic component that handle size for all text and i would then just treat icons as text. I'm curious if this PR will allow this, because passing a specific class for this to every lucide icon is cumbersome |
|
Good point |
|
Quick question: Using lucide in a react-native project, this would be the new preferred way for global styling right? Right now we have to manually provide props to each icon we use, or use a wrapper icon (which as the docs state, is not the preferred way because of importing the whole bundle) Or am I missing something? Either way thank you for your work! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 47 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
packages/lucide-react/src/Icon.tsx:51
- Missing fallback to
defaultAttributes.strokewhen bothcolorandcontextColorare undefined. Unlike React Native implementation (line 42) which includes this fallback, this could result in an undefined stroke value. For consistency and safety, add?? defaultAttributes.strokeat the end.
packages/vue/src/Icon.ts:72 - The
classprop from the parent component is missing from the class merging. The code includescontextClassbut notprops.class, which means any class passed directly to the icon component will be lost. ThemergeClassesfunction should includeprops.classto preserve classes passed to the icon.
packages/lucide-react-native/src/createLucideIcon.ts:5 - Unused imports mergeClasses, toKebabCase.
import { mergeClasses, toKebabCase, toPascalCase } from '@lucide/shared';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 47 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/vue/src/Icon.ts:72
- The
props.classis missing from themergeClassescall. The context class and generated icon classes are merged, but the component's own class prop is not included. This means any class passed directly to the icon component will be lost.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #2884
Adds context providers to globally adjust props on lucide icons.
Todo: