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

feat: New methods startOfYear and endOfYear #614

Merged
merged 5 commits into from
May 12, 2022

Conversation

flaviendelangle
Copy link
Collaborator

Goal

Add two new methods startOfYear and endOfYear following the model of the other startOfXXX / endOfXXX.

They will be used on @mui/x-date-pickers to improve the behavior when we select a year but the current date is disabled.

For instance if we are on 2018-01-01 and all days are enabled except 2019-01-01 to 2019-01-04.
If we change the year from 2018 to 2019, we expect the new date to be 2019-01-04.
But right now, it will be 2018-12-31 because we don't limit to the current year when looking for the closest enabled date.

This new method will allow to easily create the minDate / maxDate when looking for the closest valid date.

Side note

With @alexfauquette we now maintain the date pickers from MUI.
I will probably have other methods to add (getDate to get the date number in the month would be nice for instance).
If you want to organize our work differently between the adapters and the packages we are available to discuss it.

@flaviendelangle flaviendelangle changed the title feat: New methods startOfYear and endOfYear feat: New methods startOfYear and endOfYear May 10, 2022
@dmtrKovalenko
Copy link
Owner

@flaviendelangle would you like to become a collaborator on this project?

Copy link
Owner

@dmtrKovalenko dmtrKovalenko left a comment

Choose a reason for hiding this comment

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

I think it is ok to merge when you fix double clone in dayjs and make sure that build succeeds.

@@ -273,6 +273,14 @@ export default class DayjsUtils<TDate extends Dayjs = Dayjs> implements IUtils<T
return ampm === "am" ? "AM" : "PM";
};

public startOfYear = (date: Dayjs) => {
return date.clone().startOf("year") as TDate;
Copy link
Owner

Choose a reason for hiding this comment

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

I think you are doing double clone here https://day.js.org/docs/en/manipulate/start-of because start of cloning the value itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  public startOfMonth = (date: Dayjs) => {
    return date.clone().startOf("month") as TDate;
  };

We also clone for startOfMonth and startOfWeek

endOf / add also clone.
I will do a follow up PR to remove the useless clones 👍

Done for startOfYear / endOfYear

@flaviendelangle
Copy link
Collaborator Author

@flaviendelangle would you like to become a collaborator on this project?

Sure !

@dmtrKovalenko
Copy link
Owner

@flaviendelangle invited you to the project and I see some tests failed. I think once you become a collaborator you can debug everything on CI. Can you please fix them?

The changes are ok, happy to make a release when CI passes

@flaviendelangle
Copy link
Collaborator Author

Thanks for the invitation
I will have a look at the tests

@codecov-commenter
Copy link

Codecov Report

Merging #614 (df0d5a0) into master (c83bcc8) will decrease coverage by 0.26%.
The diff coverage is 87.50%.

@@             Coverage Diff             @@
##            master     #614      +/-   ##
===========================================
- Coverage   100.00%   99.73%   -0.27%     
===========================================
  Files           16       16              
  Lines         1450     1482      +32     
  Branches       186      186              
===========================================
+ Hits          1450     1478      +28     
- Misses           0        4       +4     
Impacted Files Coverage Δ
...kages/date-fns-jalali/src/date-fns-jalali-utils.ts 99.18% <50.00%> (-0.82%) ⬇️
packages/hijri/src/hijri-utils.ts 97.61% <50.00%> (-2.39%) ⬇️
packages/date-fns/src/date-fns-utils.ts 100.00% <100.00%> (ø)
packages/dayjs/src/dayjs-utils.ts 100.00% <100.00%> (ø)
packages/jalaali/src/jalaali-utils.ts 100.00% <100.00%> (ø)
packages/js-joda/src/js-joda-utils.ts 100.00% <100.00%> (ø)
packages/luxon/src/luxon-utils.ts 100.00% <100.00%> (ø)
packages/moment/src/moment-utils.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c83bcc8...df0d5a0. Read the comment docs.

@dmtrKovalenko
Copy link
Owner

Please also add a testcase or ignore to maintain 100% coverage

@flaviendelangle flaviendelangle merged commit af75b41 into dmtrKovalenko:master May 12, 2022
@flaviendelangle flaviendelangle deleted the year-boundaries branch May 12, 2022 12:01
@dmtrKovalenko
Copy link
Owner

@flaviendelangle let me know if you need to add something else or I should I release today?

@flaviendelangle
Copy link
Collaborator Author

I won't be able to use it on MUI right away (we have behaviors to clarify before)

So I can do the clonage cleaning I mentioned in a comment before releasing

This was referenced Sep 10, 2024
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.

3 participants