Skip to content
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

Relaxing propTypes for CalendarWeek #1857

Merged
merged 5 commits into from Nov 7, 2019

Conversation

@kevinthepan
Copy link
Contributor

kevinthepan commented Nov 6, 2019

Since Airbnb's own monorepo uses the wrapper PdpCalendarDay around CustomizableCalendarDay, it triggers a TypeError every time the component tree is mounted in any sort of unit test.

Relaxing the propTypes for CalendarWeek to accept any sort of node.

Kevin Pan and others added 4 commits Nov 6, 2019
Kevin Pan
Kevin Pan
Kevin Pan
Copy link
Collaborator

ljharb left a comment

This was very intentional; the fix should be made in the internal component. Why does it have to pass a custom kind of CalendarDay in the first place?

@majapw
majapw approved these changes Nov 6, 2019
@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Nov 6, 2019

The existing failure is unrelated to this change. @krissalvador27 is looking into it.

@krissalvador27

This comment has been minimized.

Copy link
Contributor

krissalvador27 commented Nov 6, 2019

Hey @majapw, I have a fix open here #1854 😁

@kevinthepan

This comment has been minimized.

Copy link
Contributor Author

kevinthepan commented Nov 6, 2019

@ljharb We have a bunch of business logic surrounding CustomizableCalendarDay (day-specific styling, day contents styling, day dependent phrases, etc) that makes things a lot cleaner and more manageable when encapsulated as part of the day instead of at the date picker level.

@majapw

This comment has been minimized.

Copy link
Contributor

majapw commented Nov 7, 2019

Thanks @krissalvador27!

And yeah, agree with @kevinthepan's statement. I think we are more generally interested in transforming this library into a more flexible solution so as to better accommodate needs both internally and externally.

@majapw majapw merged commit b600767 into airbnb:master Nov 7, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 80.724%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.