-
-
Notifications
You must be signed in to change notification settings - Fork 727
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
Don't overrides DayPickerInput selectedDays prop #531
Conversation
Thanks for the PR! Wondering why tests are failing here 🤔 |
911d0a4
to
02c607e
Compare
Codecov Report
@@ Coverage Diff @@
## master #531 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 15 15
Lines 541 553 +12
Branches 113 117 +4
=====================================
+ Hits 541 553 +12
Continue to review full report at Codecov.
|
test/daypickerinput/rendering.js
Outdated
it('should use `selectedDays` from DayPickerInput props', () => { | ||
const wrapper = mount( | ||
<DayPickerInput | ||
selectedDays={new Date(2017, 1, 8)} |
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.
I'm not sure why you are introducing a new selectedDays
prop here. The idea of this update is that it doesn't matter which day is the user clicking in the overlay: the selected days will be those coming from the dayPickerProps
if present.
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 the updated and for adding a test 🙇 ! I have a comment tough :)
test/daypickerinput/rendering.js
Outdated
wrapper.instance().showDayPicker(); | ||
wrapper.update(); | ||
expect(wrapper.find('.DayPicker-Day--selected')).toHaveLength(1); | ||
expect(wrapper.find('.DayPicker-Day--selected').at(0)).toHaveText('8'); |
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.
So this test is OK, it should have picked the selectedDays
from dayPickerProps
. No need to set selectedDays
in the <DayPickerInput />
.
The tests we are missing is:
- if
selectedDays
is passed todayPickerProps
, clicking a day in the day picker will still use the days fromdayPickerProps
- if
selectedDays
is passed todayPickerProps
, typing a new valid day in the day picker will still use the days fromdayPickerProps
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.
Oh, ok, I'll remove that extra prop, but what to do with behavior that checks by this test?
Awesome thanks a lot ! |
* Don't overrides DayPickerInput selectedDays prop * Extra selectedDays prop removed * Test added
This reverts commit ae71a0c.
This PR for #521.
dayPickerProps
moved afterselectedDays
so DayPickerInput'sselectedDays
won't be overridden.