-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
add verticalHeight prop for DayPicker #773
Conversation
Changes Unknown when pulling 12f1479 on ingiulio:daypicker_vertical_height into ** on airbnb:master**. |
12f1479
to
ef1e9d0
Compare
Changes Unknown when pulling ef1e9d0 on ingiulio:daypicker_vertical_height into ** on airbnb:master**. |
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.
Seems reasonable to me. Should we pass the prop through the DateRangePicker/SingleDatePicker components as well?
src/components/DayPicker.jsx
Outdated
@@ -55,6 +55,7 @@ const propTypes = forbidExtraProps({ | |||
hideKeyboardShortcutsPanel: PropTypes.bool, | |||
daySize: nonNegativeInteger, | |||
isRTL: PropTypes.bool, | |||
verticalHeight: PropTypes.number, |
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.
should this be a nonNegativeInteger?
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.
sounds good, I can't think of a case where it would in fact make sense to use a negative height 🤔
@majapw ok! I'll add it there too. Btw, is the |
@ingiulio try rebasing on the latest master, on the command line? |
ef1e9d0
to
6698278
Compare
6698278
to
b0eac7b
Compare
This PR will be ready to go once you can set the I also reran the pending test. |
b0eac7b
to
81912e9
Compare
Done, thank you! |
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 more examples is always a great idea! :) If we were to add a visual diffing tool like https://github.com/Galooshi/happo or something, every example we've already written would add that much more. I'm pretty happy with this.
Could you also add verticalHeight: nonNegativeInteger
to the DateRangePickerShape
and SingleDatePickerShape
files? :)
After that, I'll merge this in. :)
81912e9
to
a3964c2
Compare
a3964c2
to
8e222a7
Compare
8e222a7
to
fb1a15b
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!
Thank you! :) |
Addresses #632
👋
These changes allow the user of DayPicker to specify a vertical height (applied only when on vertical orientation), while keeping the current behavior (an arbitrary size) as the default if no height is specified.
This is actually slightly different than what the issue's title suggests, but imho it seems easier for the user to just have full control on the height rather than trying different "factors". Open to different opinions obviously!