mskocik / svelty-picker

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

Theme varibles not being used properly #153

Closed MeroVinggen closed 10 months ago

MeroVinggen commented 10 months ago

In Time.svelte component time picker styles (e.g. .sdt-middle-dot & .sdt-hand-pointer & .sdt-hand-circle & @keyframes tick-selection) are using --sdt-clock-selected-bg variable for background & border, but should use the --sdt-bg-selected according to the docs.

Coz of this it's unable to set picker color by --sdt-bg-selected, it's unused in the code so we have to use --sdt-clock-selected-bg anyway.

You may check the reproduced issue here: https://svelte.dev/repl/755a3ccc25ab41a5b150df139b459b47?version=4.2.8

I could provide a pull request to fix this.

Also, I prepose to rename --sdt-btn-header-bg-hover -> --sdt-header-btn-bg-hover for uniformity of names.

mskocik commented 10 months ago

Definitely can be... I didn't check your PR related to css\theming update (#137) in much detail.

Although I understand that --std-header-btn-bg-hover would make more sense, it would be a breaking change. If you could make it to keep working also with old prop, you can change it. For example if old prop would be a default value of the new prop, it should work.

Reflecting these variables update in the PR would need to be also addressed. But I am open to PR.

MeroVinggen commented 10 months ago

The fix of the existing variable won't make breaking changes, but renaming of --std-header-btn-bg-hover will, so we can leave it to the next major update.

mskocik commented 10 months ago

Yes, I agree. It would be perfect if you could prepare 2 PRs - one with fix, second with discussed BC break

MeroVinggen commented 10 months ago

PR with no BC: https://github.com/mskocik/svelty-picker/pull/155

MeroVinggen commented 10 months ago

I have good news, there is no BC for renaming CSS variable (e.g. --sdt-btn-header-bg-hover -> --sdt-header-btn-bg-hover)

It was already used everywhere in the code with the correct name, I guess I forgot to rename it only at the theme page in docs.

PR (no BC): https://github.com/mskocik/svelty-picker/pull/156