Skip to content

Add visibleMonth prop#1487

Closed
ianduvall wants to merge 8 commits intoreact-dates:masterfrom
ianduvall:visible-month-prop
Closed

Add visibleMonth prop#1487
ianduvall wants to merge 8 commits intoreact-dates:masterfrom
ianduvall:visible-month-prop

Conversation

@ianduvall
Copy link
Copy Markdown
Contributor

@ianduvall ianduvall commented Dec 11, 2018

This PR adds the prop visibleMonth to the components DayPicker, DayPickerRangeController, and DayPickerSingleDateController. This provides the consumer the option to control the visible month of these components.

This is an alternative to #1481 which attempts to solve #1320. The major difference is this PR adds the flexibility for the consumer to jump to any arbitrary month. This also would be backwards compatible with the prop initialVisibleMonth since the new prop visibleMonth is an opt-in override.

TODO

  • update component tests

@ianduvall
Copy link
Copy Markdown
Contributor Author

I'm opening up this PR to get some early feedback. If this approach is supported I can update the tests and make any requested improvements.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 11, 2018

Coverage Status

Coverage increased (+0.4%) to 85.56% when pulling e22d6a0 on ianduvall:visible-month-prop into d572bea on airbnb:master.

Copy link
Copy Markdown
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Overall I very much like the approach.

@ianduvall
Copy link
Copy Markdown
Contributor Author

ianduvall commented Dec 14, 2018

@ljharb I rebased off master and attempted to fix some of your suggestions. Let me know what you think.

@ianduvall
Copy link
Copy Markdown
Contributor Author

@ljharb any other comments? Otherwise I'm going to add some tests.

@ianduvall
Copy link
Copy Markdown
Contributor Author

@ljharb I wrote a few tests but definitely could use some feedback. Seems like the tests really get into the implementation details so I could def use some feedback.

@ljharb ljharb requested a review from majapw December 21, 2018 03:38
@ianduvall
Copy link
Copy Markdown
Contributor Author

@ljharb @majapw any updates?

@carllang
Copy link
Copy Markdown

Any updates on this??

@ianduvall
Copy link
Copy Markdown
Contributor Author

I'm going to close this. This doesn't seem like the best approach. In the meantime, I'm just going to remove my date picker from the DOM and then immediately add it again so that initialVisibleMonth will rerun and use an update value.

@ianduvall ianduvall closed this Jan 30, 2019
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Jan 30, 2019

@ianduvall why do you no longer think this is the best approach?

@ianduvall
Copy link
Copy Markdown
Contributor Author

The components were designed for the visible month to be controlled by the component so that it can manage animations, etc. This PR breaks that and tries to give the user control of the visible month but it creates this half user managed, half component managed approach. I think an ideal solution would treat the visible month as a user input that gets passed in like the dates and then the component would handle changing from the old visible month to the new visible month. For example, the component could animate the calendar sliding to the new visible month. But this would require a more extensive PR and a paradigm shift in how the visible month is managed.

@jchapelle
Copy link
Copy Markdown

jchapelle commented Apr 12, 2019

@ianduvall How did you manage to rerender your component completely in order to set the visible month as wanted ?

PS : too bad this PR is not finally merged ...

@pedroabreu
Copy link
Copy Markdown
Contributor

@jchapelle you can try setting the key with a date value, say the value of the input. A change in key will force a re-mount of the component so the initialVisibleMonth will kick in with the new value.

Not pretty as, in my case, there's a frame glitch... but does the job

@jchapelle
Copy link
Copy Markdown

jchapelle commented Apr 13, 2019 via email

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.

6 participants