-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Calendar get prev get next focus on same day instead of first #3355
Calendar get prev get next focus on same day instead of first #3355
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3355 +/- ##
==========================================
+ Coverage 90.9% 90.93% +0.03%
==========================================
Files 95 95
Lines 2748 2759 +11
Branches 510 512 +2
==========================================
+ Hits 2498 2509 +11
Misses 190 190
Partials 60 60
Continue to review full report at Codecov.
|
95e9652
to
36d0302
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for this!
Though, I see two changes in this PR:
- shift → alt change
- calendar
.getNext()
/.getPrev()
logic change
Could you please only leave the relevant second commit in this PR before proceeding with the review?
36d0302
to
cca081b
Compare
@benouat @maxokorokov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gpolychronisamadeus!
Thanks, it looks good. There is only one issue with the test. I left a comment.
Cheers,
Max
src/datepicker/datepicker-tools.ts
Outdated
@@ -78,11 +78,14 @@ export function generateSelectBoxYears(date: NgbDate, minDate: NgbDate, maxDate: | |||
} | |||
|
|||
export function nextMonthDisabled(calendar: NgbCalendar, date: NgbDate, maxDate: NgbDate) { | |||
return maxDate && calendar.getNext(date, 'm').after(maxDate); | |||
const nextDate = calendar.getNext(date, 'm'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, can be a bit shorter, but I have no preference really. Change if you like it
const nextDate = Object.assign(calendar.getNext(date, 'm'), {day: 1});
@@ -305,16 +305,12 @@ describe(`datepicker-tools`, () => { | |||
months = buildMonths(calendar, new NgbDate(2018, 6, 5), state, i18n, false); | |||
expect(months).toBe(state.months); | |||
expect(months.length).toBe(2); | |||
// the structures should be swapped: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this if not acceptable, because clearly the:
- behaviour is broken
- you're adapting the test to the broken behaviour
Could fix this:
const firstDate = calendar.getNext(date, 'm', i); |
I guess we should make sure that this date also always have the {day: 1}
...
Second option would be to make the tests always use dates with day 1 → buildMonths(..., new NgbDate(2018, 7, 1), ...)
, but I think I prefer the first one...
cca081b
to
f884eca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@benouat
@maxokorokov
Rebased and updated tests.
Please review.