-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Update conditions when adjustDayPickerHeight is called #446
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
Conversation
|
Can you explain the motivation for why you want to do this? Because reaching into the child component to grab a method is generally an antipattern. |
aea2784 to
877cd7e
Compare
|
Updated this PR in response to your comments in the issue. Let me know if you think that covers the possible situations when it might need to update. |
877cd7e to
811212a
Compare
|
@majapw Any further thoughts on this? I was wondering if maybe |
ljharb
left a comment
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.
This needs a rebase and some sort of test coverage.
811212a to
add3b41
Compare
|
@ljharb rebased - anything else required? what do you think of my comment above about calling |
add3b41 to
3750622
Compare
| } | ||
| } | ||
|
|
||
| if (orientation !== prevProps.orentation || daySize !== prevProps.daySize) { |
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.
Not super sure why this isn't just integrated into the above conditional? Let's make that change and then merge this in.
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 think it still needs nonzero test coverage before going in tho, if possible.
|
I've attempted to update / finish this in #640 |
|
Closing in favor of #640 |
No description provided.