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

feat(lucide-svelte) : Set defaults props with Svelte Context API #1903

Conversation

lolcabanon
Copy link

@lolcabanon lolcabanon commented Feb 20, 2024

What is the purpose of this pull request?

  • New Feature

Description

I added the use of Svelte Context API to set default icons props. This allows to set default values in any part of the app.

The first commit uses string context keys. The last one exports Symbol context keys. I tend to prefer the last one, but I am open to suggestions on naming and architecture.

WARNING :

I have not run the build or package commands yet on this pull request. I just threw this up directly on GitHub, but I have tested the code directly in my node_modules and can attest it works as expected.

I will do everything in order if you have any interest, including documentation for the lucide-svelte package. 😉

Example :

// in routes/+layout.svelte -> all the app will inherit the context
import { LucideContextIconSize } from 'lucide-svelte';

// set default width and height to 48 to the whole app
setContext(LucideContextIconSize, 48);
// in routes/nested/+page.svelte -> only set for the current page
import { LucideContextIconSize } from 'lucide-svelte';

// set default width and height to 1em in the page
setContext(LucideContextIconSize, '1em');

Why ?

The global styling with CSS is not overridable (at least in Svelte!).

This means that setting a global CSS rule will overwrite the SVG attributes and ignore them.

.lucide {
  width: 56px;
  height: 56px;
}
<LucideIcon size="48" /> <!-- THIS WILL DO NOTHING -->

Also, not being able to set default props values is not really DRY :

<LucideIcon size="1em" />

<LucideIcon size="1em" />

<LucideIcon size="1em" />

<!-- ... -->

Before Submitting

@lolcabanon
Copy link
Author

@ericfennis why are you confused?

@lolcabanon
Copy link
Author

Also see https://github.com/haruaki07/phosphor-svelte#context. Phosphor Icons has a similar feature.

@ericfennis
Copy link
Member

@lolcabanon yeah I understand the problem.
I found out it's not only lucide-svelte that has this problem.
I try to avoid using context, since it is possible to use CSS to style globally. But yeah it's not working very well if you want to override it.

But I need to think about this. We want to keep APIs similar with the other packages we have. So maybe we need to consider this for the other packages as well..

@lolcabanon
Copy link
Author

I found out it's not only lucide-svelte that has this problem.

I have used lucide only with svelte and directly as a font so I don't know about other frameworks...

I see 3 possibilities (there are probably more) :

  1. Give the end user tools (context keys) to easily manage the context themselves. (like my pull request)
  2. Export a component to "wrap" around a context. (like the Phosphor example)
  3. Initialize the lucide module with some default values. (makes me think about i18next initialization)

@karsa-mistmere karsa-mistmere changed the title Feature (lucide-svelte) : Set defaults props with Svelte Context API feat(lucide-svelte) : Set defaults props with Svelte Context API Mar 6, 2024
@HassanAzzam
Copy link

This is happening in React too. Global styling is overriding local props passed to the icon.

image

@ericfennis ericfennis changed the base branch from main to next November 29, 2024 08:19
Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

I'm going to merge this into the next branch. This will be used in the future version of @lucide/svelte

@ericfennis ericfennis marked this pull request as ready for review November 29, 2024 08:22
@ericfennis ericfennis merged commit 9ce1668 into lucide-icons:next Nov 29, 2024
2 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants