ZestfulNation / vue-hotel-datepicker

A VueJS v2 responsive date range picker. Displays the number of nights selected and allow several useful options like custom check-in/check-out rules, localization support and more.
https://zestfulnation.github.io/vue-hotel-datepicker/
MIT License
840 stars 220 forks source link

Refactor disabledDaysOfWeek #235

Closed superbiche closed 3 years ago

superbiche commented 4 years ago

As @matiasperrone suggested in #232 , disabledDaysOfWeek are currently based on the translated day name, which doesn't make sense and is pretty unstable stuff.

Suggestions (comment if you don't agree with some):

While this seems like a small change, we should make another, much bigger change: base everything on integer-based weekdays, so we don't keep 2 logics with possibly different behaviors (one with integers and one with day names like sundayToSunday stuff)

If you agree on this, I think we have the following options:

Open to suggestions !

@matiasperrone @mariusa @krystalcampioni

mariusa commented 4 years ago

First, thanks for working on this!

don't change anything, then sunday and other English day names become string IDs for weekdays, and 0 always resolves to sunday

I vote for this variant, to avoid the extra work. Maybe in future we'll see some other options to handle this, eg sundayToSunday -> minWeekDay="0", maxWeekDay="0" (I can't find docs on what exactly sundayToSunday does, only the tooltip)

krystalcampioni commented 4 years ago

Thanks for looking into this! My only concern with this is that in some countries, Sunday is not the first day of the week, Monday is. I can see this becoming a source of confusion specially if we start using cryptic names as 0to0. Maybe we should keep long week names but differentiate what is a translated string, and what is an identifier for a week day?

for example:

// en:
{ monday: "Monday" }

// pt-br:
{ monday: "Segunda-feira"}
mariusa commented 4 years ago

My only concern with this is that in some countries, Sunday is not the first day of the week, Monday is.

Mon is 1st day of week for me too. But his has been solved by dayjs, momentjs... As you said, Sun = 0, Mon = 1.

matiasperrone commented 3 years ago

We could use constants as well so... it could be strings or integers.

Be aware that MomentJS has been deprecated and the same folks created Luxon.

I also would like to change fecha package to this new one, because the community is bigger in luxon or even moment.

superbiche commented 3 years ago

I use Luxon, it's cool but a bit heavy, adds quite the load to the final bundle. Dayjs is much more lightweight, I'm switching a huge project from Luxon to Dayjs and it's really cool.

krystalcampioni commented 3 years ago

As long as we keep the reference to a the english name of the day, I'm fine either way. 👍🏽

If we were to move away from fecha, we should consider https://date-fns.org/. It has a huge community, its lightweight and well maintained.

mariusa commented 3 years ago

+1 for dayjs or date-fns

matiasperrone commented 3 years ago

So Dayjs seems easier to learn but date-fns seems more powerful, doesn't?

matiasperrone commented 3 years ago

https://www.npmtrends.com/date-fns-vs-dayjs-vs-luxon-vs-moment dayjs has less issues... although date-fns has the more downloads.... way more... 8.17M (date-fns) vs 2.92 (dayjs)...

I'll go with date-fns, the only thing is that it has more more issues and it's quite bigger.

matiasperrone commented 3 years ago

A closer look to the issues and the docs, I prefer dayjs the repository is "cleaner" and I think @superbiche and I are familiar with Moment so the change would be pretty easy.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

matiasperrone commented 3 years ago

I finally have some time:

New prop disabledWeekDays: An object with the following properties: sunday, monday, tuesday, wednesday, thursday, friday, saturday, the value indicates if that day is disabled (true) or enabled (false).

disabledDaysOfWeek still works but is deprecated. seems that this prop depends on fecha.format when the week name is returned, also it depends on the browser timezone offset, I solved this issue.

Doc updated too!