mskocik / svelty-picker

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

Theme header variables #118

Closed MeroVinggen closed 1 year ago

MeroVinggen commented 1 year ago

Provide pls css variable for header text color

Right now there is " --sdt-btn-header-bg-hover" but no such thing as " --sdt-btn-header" to set the text color

Header for text color is using "--sdt-color", that cause issue

Huge amount of colors & color variants are unable to use, like in example below, when you need same color for text & background (header text is visible only on hover)

image

mskocik commented 1 year ago

Sound reasonable. Any other customizable property is missing?

MeroVinggen commented 1 year ago

We could male use of:

"--sdt-radius" for setup correct wrap border-radius, according to project style guide

MeroVinggen commented 1 year ago

As far as I see it now, that's all, so the issue can be closed untill it needed

mskocik commented 1 year ago

Added props:

MeroVinggen commented 1 year ago

Part of the previous problem remains

header colon is using --sdt-color instead of --sdt-header-color

that's why it will become invisible in all cases --sdt-color & --sdt-primary are equal

image

mskocik commented 1 year ago

Oh, forgot about that one - fixed now.

Btw you seem to use just time picker, not date picker right? Because it would be impossible to style it properly with your setup until now (4.1.1), where I added --sdt-color-time prop for customizing color of hours/minutes for time picker

MeroVinggen commented 1 year ago

I check it out, it works good for me

Also I tried --sdt-color-time, that's usefull improvement too

MeroVinggen commented 1 year ago

--sdt-btn-bg-hover works for the date picker but not for the time picker.

MeroVinggen commented 1 year ago

Also, it would be great to add description comments for CSS variables at "https://mskocik.github.io/svelty-picker/theme".

Some of them are clear to read, but the others like --sdt-color & --sdt-primary are confusing and you have no choice but to try them all out and figure out who is who.

mskocik commented 1 year ago

I agree. Feel free to create PR with those comments, I could use some help with documentation, you know... it's harder for me as an author describe everything clearly enough.

Maybe even the design of those properties can be better. I am open to improvements.

On 10. 9. 2023 13:16, Mero wrote:

Also, it would be great to add description comments for CSS variables at <a href="https://mskocik.github.io/svelty-picker/theme'> https://mskocik.github.io/svelty-picker/theme'%3E docs: theme .

Some of them are clear to read, but the others like |--sdt-color| & |--sdt-primary| are confusing and you have no choice but to try them all out and figure out who is who.

Message ID: @.***>

MeroVinggen commented 1 year ago

I could try and so I found some to discuss.

Bugs:

  1. --sdt-today-bg is used as hover, not as bg and so the real bg is cumming from --sdt-primary, but have no css var for it (In your CSS vars list). I think it should be renamed to --sdt-today-bg-hover and so add --sdt-today-bg for setting buttons bg
  2. no CSS var for --sdt-clear-hover-bg but is used maroon

Improvements:

  1. --sdt-btn-bg-hover should be renamed (perhaps to --sdt--pick-item-bg-hover). Also it should be used for time picker same styling purposes.

After we finish with Bugs & Improvements the names of vars from list below could be also a bit changed.

Comments:

--sdt-bg-main: white; /** wrap background color */
--sdt-shadow-color: lime; /** wrap shadow color */
--sdt-wrap-shadow: 0 1px 6px var(--sdt-shadow-color); /** wrap shadow settings */
--sdt-radius: 14px; /** wrap radius */
--sdt-color: aqua; /** items to pick text color (except header & buttons) */
--sdt-color-selected: green; /** selected item text color */
--sdt-color-time: pink; /** optionally separate color for time picker (watch "--sdt-color") */
--sdt-header-color: orange; /** header items color (e.g. text & buttons) */
--sdt-primary: lightgrey; /** chosen item color */
--sdt-disabled-date:  var(--sdt-bg-main); /** disabled dates text color */
--sdt-disabled-date-bg: black; /** disabled dates background color */
--sdt-btn-bg-hover: tomato; /** items to pick background color */
--sdt-btn-header-bg-hover: gold; /** header items hover background color */
--sdt-today-indicator: red; /** date picker current day marker color */
--sdt-clock-bg: grey; /** time picker inner circle background color */
--sdt-table-bg: green; /** date picker inner table background color */
/* action buttons */
--sdt-today-bg: grey; /** date picker today button hover background color */
--sdt-today-color: green; /** date picker today button text & border color */
--sdt-clear-color: purple; /** clear button text & border color */
--sdt-clear-bg: pink; /** clear button background color */
--sdt-clear-hover-color: var(--sdt-bg-main); /** clear button hover text color */

At this one I need your opinion & possible changes

mskocik commented 1 year ago

About the bugs:

  1. Yes, you are right
  2. This is intentional, default value is not set, but fallback value is provided - so this is not a bug
  3. selected today indicator is hard coded to #eee

Improvements:

  1. Yes, it should be renamed to --sdt-table-btn-bg-hover to make it clea is related to date view
  2. Also values for selected button (color, bg) should be split (I think) between date and time picker to allow better separation (they both use --sdt-primary, which will be useless at that point, but can be their fallback value

Edit: Your thoughts?

MeroVinggen commented 1 year ago

About the bugs:

About clear button bg hover, yeah, I see clearBtnClasses prop, but... It doesn't look as a good design to me. I mean, you provide a prop & CSS vars and this make a small confuse.

If you hardly don't want to add --sdt-clear-hover-bg var(which I recommend you to do), no problem, we can at least add comment about it, like "no CSS var for this one, but you can overwrite it's behavior by prop... look at... ".

Improvements:

I agree about split for selected

Comments:

Do you have any changes to the comments I suggest?

mskocik commented 1 year ago

About bugs:

I would like to keep (and extend a little bit) ability to pass classes as props (as is suggested in #132 as preferred way to inject styles). But it makes sense (at least for me) to have the option to quickly customize some props if you need to, but in overall you are satisfied with "defaults".

I am not sure I fully understand what you say about --sdt-clear-hover-bg. This property _IS_defined, so it is fully customizable. Only the default value is missing.

Comments:

--sdt-btn-bg-hover: tomato; /** items to pick background color */

This is not fully clear that it means "date picker day button". Otherwise the rest make sense to me.

MeroVinggen commented 1 year ago

I guess not I got you, the prop --std-clear-hover-bg exists, it's just not mentioned in CSS props list. I checked and it truly exists. So I will just add it in to the list.

I am not sure I fully understand what you say about --sdt-clear-hover-bg. This property _IS_defined, so it is fully customizable. Only the default value is missing.

I agree about --sdt-btn-bg-hover, I guess we should at first split it for date picker & time picker, it's useless now to update prop, which we gonna replace with the newest two.

Is it gonna be --sdt-clock-data-bg-hover & --sdt-table-data-bg-hover?

And their comments:

--sdt-clock-data-bg-hover: ...; /** clock selection data hover background color */
--sdt-table-data-bg-hover: ...; /** table selection data hover background color */
mskocik commented 1 year ago

I guess not I got you, the prop --std-clear-hover-bg exists, it's just not mentioned in CSS props list. I checked and it truly exists. So I will just add it in to the list.

Yes, I must have missed it

I agree with the rest

MeroVinggen commented 1 year ago

--sdt-color-selected has been duplicated in the list, which value should I leave to it? (#fff or var(--sdt-bg-main))

Also I propose split --sdt-primary into to variables for clock and table: -sdt-clock-selected-bg & -sdt-table-selected-bg

Comments update:

--sdt-color: #000; /** items to pick data to select text color (except header & buttons) */

mskocik commented 1 year ago

Keep the --sdt-color-seleced as #fff because it's not the same in dark mode in example.

Yes split it, but use --sdt-primary as fallback value, so user can set only one color if needed.

comment update: date instead of data, otherwise ok

MeroVinggen commented 1 year ago

I guess I finished, but need your approvement/corrections.

And here one more thing:

About the other stuff we discussed, I did a few changes to comments & variables names.

Final version:

/* general */
  --sdt-bg-main: #fff; /** wrap background color */
  --sdt-shadow-color: #ccc; /** wrap shadow color */
  --sdt-wrap-shadow: 0 1px 6px var(--sdt-shadow-color); /** wrap shadow settings */
  --sdt-radius: 4px; /** wrap radius */
  --sdt-color: #000; /** data to select(e.g date/time) text color (except header & buttons) */
  --sdt-color-selected: #fff; /** selected data(e.g date/time) text color */
  --sdt-header-color: #000; /** header items color (e.g. text & buttons) */
  --sdt-btn-header-bg-hover: #dfdfdf; /** header items hover background color */
  --sdt-primary: #286090; /** selected data(e.g date/time) background color */

  /* action buttons */
  --sdt-today-bg: #1e486d; /** date picker today button hover background color */
  --sdt-today-color: var(--sdt-bg-main); /** date picker today button text & border color */
  --sdt-clear-color: #dc3545; /** clear button text & border color */
  --sdt-clear-bg: transparent; /** clear button background color */
  --sdt-clear-hover-color: var(--sdt-bg-main); /** clear button hover text color */
  --sdt-clear-hover-bg: #dc3545; /** clear button hover background color */

  /* time picker */
  --sdt-clock-selected-bg: var(--sdt-primary); /** selected time background color */
  --sdt-clock-bg: #eeeded; /** time picker inner circle background color */
  --sdt-clock-color: var(--sdt-color); /** time picker text color (watch "--sdt-color") */
  --sdt-clock-color-hover: var(--sdt-color); /** time picker hover text color (watch "--sdt-color") */
  --sdt-clock-time-bg: transparent; /** time picker time background color */
  --sdt-clock-time-bg-hover: transparent; /** time picker time selection hover background color */
  --sdt-clock-disabled-time: #b22222; /** disabled time picker time text color */
  --sdt-clock-disabled-time-bg: #eee; /** disabled time picker time background color */

  /* date picker */
  --sdt-table-selected-bg: var(--sdt-primary); /** selected date background color */
  --sdt-table-disabled-date: #b22222; /** disabled dates text color */
  --sdt-table-disabled-date-bg: #eee; /** disabled dates background color */
  --sdt-table-bg: transparent; /** date picker inner table background color */
  --sdt-table-data-bg-hover: #eee; /** table selection data hover background color */
  --sdt-table-today-indicator: #ccc; /** date picker current day marker color */
mskocik commented 1 year ago
  • how about rename --sdt-primary into --sdt-bg-selected for matching to existed --sdt-color-selected

I agree. The rest also looks good to me 👍