Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use datelibrary to compare months #429

Merged

Conversation

eltonvus
Copy link
Contributor

Nowadays we rely on JavaScript dates when checking whether or not a day belongs to the same month. Which leads us to a problem, because depending on the timezone configured, we can jump or be behind by a day.

However, when using momentjs.com/timezone/, it is necessary to rely on dateLibrary, to obtain the currentMonth instead of using the JavaScript date.

Copy link
Collaborator

@mkszepp mkszepp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@mkszepp mkszepp merged commit 9126429 into cibernox:master Mar 15, 2024
15 checks passed
@mkszepp mkszepp added the bug label Mar 15, 2024
@mkszepp
Copy link
Collaborator

mkszepp commented Mar 15, 2024

released in v1.0.1

@Techn1x
Copy link
Contributor

Techn1x commented Apr 4, 2024

Just an FYI - this will cause issues in the unmaintained plugin ember-power-calendar-date-fns - which only caters for "day" usage of isSame() from dateLibrary

https://github.com/makepanic/ember-power-calendar-date-fns/blob/441c403c4d838a454c7f62e07a803a25e3cf76d0/addon/unlocalized.js#L113-L120

That's not a problem for this package, but it may be reported as an issue. The error looks like this

global failure: unsupported parameter: isSame(Thu Apr 04 2024 00:49:44 GMT+0000 (Coordinated Universal Time),month)

(pasted here to help others with search)

I use a local copy of the date-fns plugin util anyway, so I've justed updated my local copy of isSame()

@miguelcobain
Copy link
Contributor

@Techn1x I think that's a bug on ember-power-calendar-date-fns, IMO.

The unit argument is there, so it should work for any unit.
Perhaps you could PR your fix there?

@Techn1x
Copy link
Contributor

Techn1x commented Apr 4, 2024

I think that's a bug on ember-power-calendar-date-fns, IMO.

yes that's what I meant by That's not a problem for this package

The unit argument is there, so it should work for any unit.
Perhaps you could PR your fix there?

The problem is the maintainer is unresponsive and we can't get anything merged there, eg makepanic/ember-power-calendar-date-fns#54

My comment was to highlight that the unmaintained plugin is now going to have issues.

@miguelcobain
Copy link
Contributor

I see. That's unfortunate. Useful to have this info here then. 👍

@Techn1x
Copy link
Contributor

Techn1x commented Apr 4, 2024

It's probably overdue for a fork. Maybe I'll do it at some point if nobody else does

@mkszepp
Copy link
Collaborator

mkszepp commented Apr 4, 2024

@miguelcobain @Techn1x thanks for this info. Unlucky that we are running inside an unsupported case after this change :(

As the other packages is not anymore maintained and the maintainer doesn't response, i think the best solution is to bring the ember-power-calendar-date-fns as extra package inside this repo and than we publish it with an other scope. So we have every future changes in our hand.
While porting the meta pakages, we should convert date-fns meta packages also to TS, so everything is modernized like the rest.

Also our other meta packages like moment and luxon package we should transfer.

After this migration we have the most used meta packages all in one place and we can give support to every package...

I will look to find time in next days to to that (atm i'm a little bit busy)

@mkszepp
Copy link
Collaborator

mkszepp commented Apr 7, 2024

@Techn1x for the moment i have extended / updated my fork (makepanic/ember-power-calendar-date-fns#54)

Changes:

  • Update date-fns to v3
  • extend isSame

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants