mskocik / svelty-picker

Simple date & time picker in svelte
https://mskocik.github.io/svelty-picker/
202 stars 46 forks source link

Ok and Cancel button classes #132

Closed rakuzen25 closed 1 year ago

rakuzen25 commented 1 year ago

Hi! Thank you so much for maintaining this useful library.

I am trying to customise the looks of the buttons for the date picker, but it seems like todayBtnClasses and clearBtnClasses only got applied to Today and Clear buttons, even though Ok and Cancel buttons share the same classes as them.

https://github.com/mskocik/svelty-picker/blob/b665db1328de61fdaee3b4754e6fbef59629a9b2/src/lib/components/SveltyPicker.svelte#L613-L622

Should these classes also be applied to Ok and Cancel?

mskocik commented 1 year ago

Should these classes also be applied to Ok and Cancel?

Not really, I planned to remove those "class" props for v4, but I forgot about it - because it was copy & paste from old code.

I would suggest look update by CSS properties. Or isn't it enough? Would you prefer passing those props to Ok and Cancel buttons? I don't really have an opinion which solution is better.

rakuzen25 commented 1 year ago

Should these classes also be applied to Ok and Cancel?

Not really, I planned to remove those "class" props for v4, but I forgot about it - because it was copy & paste from old code.

I would suggest look update by CSS properties. Or isn't it enough? Would you prefer passing those props to Ok and Cancel buttons? I don't really have an opinion which solution is better.

I don't really like using site-wide CSS directly because I often have to find myself resorting to !important or doubling the class name to override the specificity ranking. Plus, when I'm using a library, I'd have to copy over the definition of the library-provided classes (e.g. a button class).

IMMHO having the classes as a passable prop (and a global config option) feels more natural, as if implementing it in HTML myself.

MeroVinggen commented 1 year ago

I agree, props would be much cleaner and easier to use.

With CSS vars we have to make some messy stuff by inserting them into the wrap markup, like:

<div
  class="..."
  style={`
    --sdt-bg-main: ${passedCSSVaueAsProp1};
    --sdt-color: ${passedCSSVaueAsProp2};
    --sdt-primary: ${passedCSSVaueAsProp2};
    ...
  `}
>
  <SveltyPicker ... />
</div>
mskocik commented 1 year ago

With CSS vars we have to make some messy stuff by inserting them into the wrap markup, like:

BTW You can pass them directly to the component like. Of course it's ugly as well when you want to define multiple props.

<SveltyPicker --sdt-btn-bg-hover="red" />
MeroVinggen commented 1 year ago

With CSS vars we have to make some messy stuff by inserting them into the wrap markup, like:

BTW You can pass them directly to the component like. Of course it's ugly as well when you want to define multiple props.

<SveltyPicker --sdt-btn-bg-hover="red" />

I think you should add this example to Properties page so to make it possible to know about this feature.

mskocik commented 1 year ago

With CSS vars we have to make some messy stuff by inserting them into the wrap markup, like:

BTW You can pass them directly to the component like. Of course it's ugly as well when you want to define multiple props.

<SveltyPicker --sdt-btn-bg-hover="red" />

I think you should add this example to Properties page so to make it possible to know about this feature.

This is standard svelte feature, I don't think it should be mentioned in the docs.

MeroVinggen commented 1 year ago

With CSS vars we have to make some messy stuff by inserting them into the wrap markup, like:

BTW You can pass them directly to the component like. Of course it's ugly as well when you want to define multiple props.

<SveltyPicker --sdt-btn-bg-hover="red" />

I think you should add this example to Properties page so to make it possible to know about this feature.

This is standard svelte feature, I don't think it should be mentioned in the docs.

Yes, it's a standard feature but it doesn't make sense if you as a developer are not carrying these extra props.

To check this, the user could only make a guess and try it or inspect the package code.

In the library case no one guarantees you could use this feature or not and how exactly it would work in this case (what can you pass, how it would be handled, what rules you should be aware and so on, that's all is only your particular implementation of it, which could be skipped).

mskocik commented 1 year ago

To check this, the user could only make a guess and try it or inspect the package code.

In the library case no one guarantees you could use this feature or not and how exactly it would work in this case (what can you pass, how it would be handled, what rules you should be aware and so on, that's all is only your particular implementation of it, which could be skipped).

That's why CSS props are documented in the docs. So everyone can check what props are available. Or do you think, it's not obvious?

mskocik commented 1 year ago

I don't really like using site-wide CSS directly because I often have to find myself resorting to !important or doubling the class name to override the specificity ranking. Plus, when I'm using a library, I'd have to copy over the definition of the library-provided classes (e.g. a button class).

IMMHO having the classes as a passable prop (and a global config option) feels more natural, as if implementing it in HTML myself.

@leranjun not sure how to achieve this - if I allow passing theme classes as props, I can't define decent 'look' by default, because I would need to mark default classes as :global(), because they would not be present in the code. Do you have any experience if it's possible? For example vkurko/calendar is doing it like this, but he provides external css which is expected to be imported.

MeroVinggen commented 1 year ago

CSS

Didn't find there mentions about CSS vars so better to add them.

rakuzen25 commented 1 year ago

if I allow passing theme classes as props, I can't define decent 'look' by default, because I would need to mark default classes as :global(), because they would not be present in the code

Sorry, I'm not too sure what you mean by this? Why can't the default classes stay in the code?