WickyNilliams / cally

Small, feature-rich calendar components
https://wicky.nillia.ms/cally/
MIT License
1.06k stars 12 forks source link

Calendar representation for value in `CalendarDate` appears very wrong #12

Closed atmgrifter00 closed 2 months ago

atmgrifter00 commented 2 months ago

On your example page (and even in my local exploration) the CalendarMonth widget is not being correctly represented in a couple of ways: 1) The highlighted day specified by the value property is a month behind. In you example, value is specified as "2024-01-10" (January 10th 2024), but the highlighted day is December 10th, 2023. I see the same inconsistency locally. 2) Perhaps worse, is that the days do not begin on the proper day for the displayed month and year. On your example page, the month of December in 2023 is starting on Sunday, when it should be Friday. I'll note that the month of 12/24 does start on a Sunday.

WickyNilliams commented 2 months ago

Thanks. Can you give some further information to help me out? What browser, version, and OS? What is your system locale?

Seems like there's an off by one error from your description, which is a classic error for JS Dates since months are zero based.

However, everything looks fine to me. I've tested on Firefox and chrome on my phone.

Screenshot_20240404-105523

Perhaps I've misunderstood.

atmgrifter00 commented 2 months ago

I see this in both Firefox and Chrome (using Windows 10 with lang set to en): image

If I find some time I might try and pull down your source and debug it locally to see if I can find what's going wrong. Actually seems like this could be a promising fit for our use-case!

WickyNilliams commented 2 months ago

Interesting. There was a bug in the <calendar-range> page (which i just fixed here https://github.com/WickyNilliams/cally/pull/15/commits/be6d2e9b1c5624672d22e3911cf29b7160de36f9), but I'm not seeing this for <calendar-date>. I've tested in windows 11 with edge, FF, and chrome

atmgrifter00 commented 2 months ago

I'll check with others on my team to see if they see the same issue.

kswedberg commented 2 months ago

fwiw, I'm seeing this issue, as well, on your demo page at https://wicky.nillia.ms/cally/guides/theming/ (using a Chromium-flavored browser on MacOS). Here is a screenshot

image
WickyNilliams commented 2 months ago

Thanks, good to have a second confirmation. I'm not able to reproduce in either case. It looks like the issue is the headings are incorrect rather than the month itself.

What do you get if you log in the console:

console.log(navigator.languages[0])
WickyNilliams commented 2 months ago

But hmm it explicitly sets the the locale there, so that shouldn't matter. Interested to hear anyway

kswedberg commented 2 months ago

I get en-US. Not sure where you're located geographically, but it could have something to do with timezone offsets. For example, if I log the result of const date = new Date(Date.UTC(2024, 1 - 1, 1)) (from https://github.com/WickyNilliams/cally/blob/main/src/utils/temporal.ts#L21), I get "Sun Dec 31 2023 19:00:00 GMT-0500 (Eastern Standard Time)".

If I then log new Intl.DateTimeFormat('en-GB').format(date), I get "31/12/2023"

WickyNilliams commented 2 months ago

That's got to be it! Thanks for digging in. I wonder if I explicitly set the timeZone option of the DateTimeFormat to UTC it'll fix it.

kswedberg commented 2 months ago

Yes, that should do it: new Intl.DateTimeFormat('en-GB', {timeZone: 'UTC'}).format(date) → "01/01/2024"

WickyNilliams commented 2 months ago

OK, i just merged a PR which uses UTC timeZone across the board. The docs actually use a local build of the lib (not the npm version). So before I publish to npm, can you confirm whether this has fixed the issue? https://wicky.nillia.ms/cally/components/calendar-date/

kswedberg commented 2 months ago

Yes, LGTM!

WickyNilliams commented 2 months ago

Released as 0.4.2. Thanks so much for your help

atmgrifter00 commented 2 months ago

So, not to rain on the parade, but when I go to https://wicky.nillia.ms/cally/components/calendar-date/, I see: image

and while it's now correctly selecting the expected day of the displayed year, the days do not begin on the proper day of the week. January 1st 2024 is on a Monday, which I can see is how your picture you posted looks, but when your display starts the day of the week on Sunday, it looks like it gets goofed up.

WickyNilliams commented 2 months ago

😂 Damn it

I think I know the cause. I'll fix tomorrow and reopen in the meanwhile

WickyNilliams commented 2 months ago

OK, i have merged another PR. Now all instances of Intl.DateTimeFormat use the "UTC" timezone. Can you let me know if this has resolved the issue?

atmgrifter00 commented 2 months ago

Appears to have fixed it. Seems the requirement (as the API suggests) is that if you want the week to start on Sunday, you must set the first-day-of-week attribute to 0. Prior to your change it looked like it was figuring that out from the browser locale?

WickyNilliams commented 2 months ago

It was the same bug as before, just a different manifestation.

I use DateTimeFormat to get day names based on locale. But the user's timezone could introduce drift in the output. For the same reason it output new Date(2024, 0, 1) as December due to timezone differences, it treated a Monday as a Sunday and so on. Now that I fix the timezone to UTC, it won't get affected by user's timezone.

But yes, you just configure first day of week. I think you can get this info from the browser, but I don't think the API is widely available yet.

WickyNilliams commented 2 months ago

Published as 0.4.3. Will close this now. Hoping that's the last of the issues here :)