fix: correctly disabled/enabled addOffset propsGetter with maxDate using the offsetValue #51
Conversation
There was a problem hiding this comment.
@susosamuel Great job! Thank you for your contribution
Left several comments and suggestion.
Also please bump version to 6.6.0 here https://github.com/rehookify/datepicker/blob/main/packages/datepicker/package.json#L3
| expect(result.current.propGetters.addOffset({ months: 1 }).disabled).toBe( | ||
| true, | ||
| ); | ||
| expect( | ||
| result.current.propGetters.subtractOffset({ months: 1 }).disabled, | ||
| ).toBe(true); | ||
| ).toBe(undefined); |
There was a problem hiding this comment.
The only thing I would like to see here is to get to the point when this become false. So you need to perform one extra click and maybe reload the state.
There was a problem hiding this comment.
I implemented the points from the other comments, but I'm struggling with this one. I'm not sure if I understand your goal correctly and at the same time I may be not proficient enough in the lib/test suite functionality to be able to implement this. Please can you give some guidance on how to do this ?
| { days, months, years }: DPOffsetValue, | ||
| dateEdge?: Date, | ||
| ): Date => { | ||
| if (dateEdge) { |
There was a problem hiding this comment.
I would prefer return first approach here
| if (dateEdge) { | |
| if (!dateEdge) return offsetDate; |
It will reduce 1 lvl of ifs
| if (days && days !== 0) { | ||
| return calculateNewDateWithOffset(offsetDate, dateEdge, days, 'date'); | ||
| } | ||
| if (months && months !== 0) { | ||
| return calculateNewDateWithOffset(offsetDate, dateEdge, months, 'month'); | ||
| } | ||
| if (years && years !== 0) { | ||
| return calculateNewDateWithOffset(offsetDate, dateEdge, years, 'year'); | ||
| } |
There was a problem hiding this comment.
Here we can simplify by adding default params during destructuring at the params
{ days = 0, months = 0, years = 0 }: DPOffsetValue,
Then your check will shrink to if (days > 0), if (month > 0) ...
There was a problem hiding this comment.
@susosamuel Thanks so much for the fix. @Feshchenko, thanks for the release. It was perfecting timing, thanks to both of you, for my project! There is one little bug that needs to be addressed from this fix. We should also check on the edge if it is either first or last in the array because now the first and last months in the calendar are enabled then after the click event are disabled. My project has asked to fix this in low priority. I can do an external fix in my UI but it would best to handle it in the library. I can make a PR in the coming weeks to fix this small bug.
Hello, This is my attempt to fix the behaviour described in #46
I tried the approach that you, @Feshchenko, mentioned in your comments and used the offsetDate in order to achieve the desired behavior without needing to update the disabling logic.
This is an example of what the code does behind the scenes (the same happens in the videos attached below):
Situation 1 - Edge case maxDate:
Situation 2 - Edge case minDate:
-> Now, the user clicks on the previous button, what happens next:
I'm also attaching the before and after showcases
Before:
https://github.com/rehookify/datepicker/assets/17575434/2b4209a9-072c-4679-a7f5-99692b6643ea
After:
https://github.com/rehookify/datepicker/assets/17575434/f8a20c7f-8153-40e9-9b94-0e70fc558820
Props setup from the video:
mode: 'single'
maxDate: 13.2.2024
minDate: 22.11.2024
selectedDates: [20.1.2024]
fyi @marianadrozdova @ebartunek