search

mymth / vanillajs-datepicker

A vanilla JavaScript remake of bootstrap-datepicker for Bulma and other CSS frameworks
MIT License
719 stars 147 forks source link

Add $dp-font-color variable to Sass template for Foundation #170

Closed chesio closed 6 months ago

chesio commented 8 months ago

Hi,

I'm using Sass template for Foundation and would like to suggest small improvement.

The dp-button mixin directly references value of $body-font-color variable from Foundation, so there is no way how to change it for datepicker component.

My suggestion is to introduce new $dp-font-color variable and use it in the mixin:

$dp-font-color: $body-font-color !default;

This keeps the template backwards-compatible and handles $body-font-color the same way as other Foundation variables like $body-background are handled.

mymth commented 7 months ago

First, let me explain why it was made like that.

The reason why I omitted the $dp-font-color variable is that the color property inherits the value from the parent by default and thus, it automatically matches the font color of the block containing the input element (and in most cases, same as body's). The reason we have $dp-background-color is that the background-color property is transparent by default and doesn't inherit and thus, something has to be set to prevent the picker element from becoming see-through. And since the dp-button mixin is the place intended for writing framework-specific code, I considered it safe to use the framework's variables directly in there.

Next, I see a few things to consider with your code.

  1. Since $dp-background-color is for the base background color of the picker element, the name $dp-font-color implies that it's for changing the base font color of the picker element. 
However, the new variable actually changes buttons' color only. Therefore, the name $dp-font-color is misleading and better to be like $dp-button-font-color.
  2. Users' demand for customizing buttons shouldn't be only for font color. To truly allow users to customize the buttons, the change should also include the variables for background color, border, and the hover/disabled/active variants of those.
  3. When a new dp- prefixed variable is defined, it should be done in the base stylesheet (datepiceker.scss) first, then overridden in the framework variant's. This means other framework variants' stylesheets also need to be updated so that they use the variable when it's set.

And finally, I appreciate your pointing out that there is no way to customize the buttons. Now I realize the problem. However, I make it a policy not to accept requests for patch-like stuff that only treats a single immediate issue without solving its more fundamental problem. For this reason, I'm sorry, but I'd like to turn down this PR, and will add a more general solution in the future version, instead. (Of course, it will be very welcome if you update the code considering the above points, and resubmit a PR.)

Thank you for understanding.

chesio commented 6 months ago

Sorry for replying so late. Thanks for your feedback, I see your points! I'm closing this PR now - if I find some time, I might try to come up with a new PR that considers your points above.