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

Replacing some utils for date-fns functions #17

Conversation

arthurdenner
Copy link
Contributor

As suggested on #10 and #14, this PR aims to replace some functions on the utils module for date-fns equivalents. The following were replaced:

  • adjustDateByXDays by addDays;
  • isBefore by isBefore;
  • isEqual by isToday (it was used only to compare a received date with a new Date();
  • normalizeDate by startOfDay.

@arthurdenner arthurdenner changed the title Replacing some utils for date fns functions Replacing some utils for date-fns functions Apr 10, 2018
@mkartchner994
Copy link
Contributor

This is looking great. Thanks for the work!
Should we also look at using https://date-fns.org/v1.29.0/docs/differenceInMonths to replace the monthDiff function in utils?

@arthurdenner
Copy link
Contributor Author

Thanks, @mkartchner994.

I didn't replace them because they return different values, probably because differenceInMonths returns the number of full months between the given dates while monthDiff returns the difference between the .getMonth() calls. E.g:

dateFns.differenceInMonths(new Date(2014, 8, 1), new Date(2014, 0, 31)) // returns 7
// the functions have different signatures, but that's not a problem
monthDiff(new Date(2014, 0, 31), new Date(2014, 8, 1)) // returns 8

@mkartchner994
Copy link
Contributor

@arthurdenner Ah, I see. Is that the same for the differenceInCalendarMonths method? https://date-fns.org/v1.29.0/docs/differenceInCalendarMonths

@arthurdenner
Copy link
Contributor Author

I didn't see this function! And no, differenceInCalendarMonths returns 8 too. I'll commit this change.

@@ -57,7 +58,7 @@ function noop() {}
export function subtractMonth({ calendars, offset, minDate }) {
if (offset > 1 && minDate) {
let { firstDayOfMonth } = calendars[0];
let diffInMonths = monthDiff(minDate, firstDayOfMonth);
let diffInMonths = differenceInCalendarMonths(firstDayOfMonth, minDate);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While monthDiff had the signature (earlierDate, laterDate), differenceInCalendarMonths has the opposite.

@@ -77,7 +78,7 @@ export function subtractMonth({ calendars, offset, minDate }) {
export function addMonth({ calendars, offset, maxDate }) {
if (offset > 1 && maxDate) {
let { lastDayOfMonth } = calendars[calendars.length - 1];
let diffInMonths = monthDiff(lastDayOfMonth, maxDate);
let diffInMonths = differenceInCalendarMonths(maxDate, lastDayOfMonth);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as here: c6023c9#r180536919

@mkartchner994
Copy link
Contributor

@arthurdenner
This looks great. Thanks!

I think I have decided after all that date-fns should be a peer-dependency and not get bundled into the final dist output. Because of this, what I am planning on doing is bumping up to the next major version number to v2 so this won't cause anyone's v1 to break if they don't have date-fns brought into their project.

I will do some work on that tonight or tomorrow and then let you know when I cut that release.

Also, I would like recognize your work based on the "All Contributors" specification - https://github.com/kentcdodds/all-contributors. If that is alright with you, another pull request with that would be great - should just be able to run the add-contributor script in package.json. Or I can commit that on my end as well. Just let me know.

Thanks again!

@mkartchner994 mkartchner994 merged commit cccc0df into deseretdigital:master Apr 11, 2018
@arthurdenner arthurdenner deleted the refactor/replacing-some-utils-for-date-fns-functions branch April 11, 2018 12:06
@arthurdenner
Copy link
Contributor Author

arthurdenner commented Apr 11, 2018

Thanks, @mkartchner994. ❤️

Feel free to add me on the contributors list, I tried to run add-contributor but got some error related to the CLI that I couldn't solve 😅

I closed #14 and you probably want to close #10. I'll open a PR to solve #5 if that's okay.

@mkartchner994
Copy link
Contributor

@arthurdenner v2.0.1 has bee published. Thanks for your work on this. I have also added you to the contributors list 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants