bestguy / sveltestrap

Bootstrap 4 & 5 components for Svelte
https://sveltestrap.js.org
MIT License
1.3k stars 183 forks source link

Should `color` prop type definitions allow arbitrary string values? #453

Open ohmree opened 2 years ago

ohmree commented 2 years ago

With the following sass that generates a new button variant:

// src/app.scss
// include this however you like.
@import 'bootstrap';

$twitch-purple: #9146ff;
.btn-twitch {
  @include button-variant($twitch-purple, $twitch-purple);
}

I can do <Button color="twitch" /> and get the expected behavior (a purple button) - except typescript complains about 'twitch' not being a valid value for this prop. Should components that accept color/variant props allow arbitrary strings for cases like this? There are other ways to make the button look the way I want it to in this case but I think they're both inferior:

bestguy commented 2 years ago

Hi @ohmree , Yes I think we should support | string here: https://github.com/bestguy/sveltestrap/blob/master/src/shared.d.ts#L8 since Bootstrap allows this type of customization.

Thanks for the heads up, we can PR this week.

demetrius-mp commented 2 years ago

@bestguy actually it would be necessary to change the color on the Button.d.ts file as well. I think it would be better if the ButtonColor type kinda "extended" the Color type available in shared.d.ts, as it only adds another color (which is "link").

demetrius-mp commented 2 years ago

@bestguy also, i just tried adding | string to the type definition and it completely loses intellisense, and at the end of day, it will treat the type as a simple string, not literals

ohmree commented 2 years ago

i just tried adding | string to the type definition and it completely loses intellisense, and at the end of day, it will treat the type as a simple string, not literals

I've noticed that too, I think it's just due to how typescript works.

What I typically end up doing is using my editor's go-to-definition functionality to check what the type looks like.
It's not ideal but it works.

EDIT: I asked on the typescript discord and was told that the workaround for this is something along the lines of 'variant1' | 'variant2' | string & {}, but the person who suggested this said they try to avoid this pattern when possible.

demetrius-mp commented 2 years ago

EDIT: I asked on the typescript discord and was told that the workaround for this is something along the lines of 'variant1' | 'variant2' | string & {}.

just tested this and it works just as expected. still providing intellisense for the literal strings, but allowing to enter other values.

but the person who suggested this said they try to avoid this pattern when possible.

sure, this syntax definetly looks weird, but i guess there is no other way to handle this without sacrificing intellisense

ohmree commented 2 years ago

Another alternative that was suggested is 'variant1' | 'variant2' | { custom: string }. I'm not sure which one is preferable here but I think I'm leaning more towards the former version (| string & {}), just because it looks nicer.