-
-
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 verticalSpacing prop #1096
add verticalSpacing prop #1096
Conversation
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 multiple flat props is better than an object/shape
src/components/DayPicker.jsx
Outdated
@@ -66,6 +66,7 @@ const propTypes = forbidExtraProps({ | |||
verticalHeight: nonNegativeInteger, | |||
noBorder: PropTypes.bool, | |||
transitionDuration: nonNegativeInteger, | |||
verticalSpacing: 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 non-negative, and an integer?
@@ -66,6 +66,7 @@ const propTypes = forbidExtraProps({ | |||
hideKeyboardShortcutsPanel: PropTypes.bool, | |||
daySize: nonNegativeInteger, | |||
noBorder: PropTypes.bool, | |||
verticalSpacing: 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.
also here
daeebe8
to
a187fe3
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.
I'm concerned about overloading this naming in the library.
src/components/CalendarMonth.jsx
Outdated
@@ -45,6 +45,7 @@ const propTypes = forbidExtraProps({ | |||
renderDayContents: PropTypes.func, | |||
firstDayOfWeek: DayOfWeekShape, | |||
setMonthHeight: PropTypes.func, | |||
verticalSpacing: 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.
I think this name is somewhat overloaded as verticalSpacing
in the case of the SDP and DRP signals the margin between the calendar dropdown and the inputs. Could we name this either verticalBorderSpacing
to allude to the table or verticalWeekSpacing
to allude to the calendar contents?
src/components/CalendarMonth.jsx
Outdated
{...css( | ||
!verticalSpacing && styles.CalendarMonth_table, | ||
verticalSpacing && styles.CalendarMonth_verticalSpacing, | ||
{ borderSpacing: `0px ${verticalSpacing}px` }, |
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.
Does this get weird when the verticalSpacing is undefined? Like it would try to apply a style of 0px undefinedpx
and then it would crap out. I think it would be better to do:
verticalSpacing && {
borderCollapse: 'separate',
borderSpacing: `0px ${verticalSpacing}px`,
},
or
verticalSpacing && styles.CalendarMonth_verticalSpacing,
verticalSpacing && { borderSpacing: `0px ${verticalSpacing}px` },
a187fe3
to
c7ec725
Compare
@majapw updated |
Sweet! Looks great to me! Will merge in and do a release. |
This adds a verticalSpacing prop to DayPickerRangeController, which is passed all the way to
CalendarMonth
. This will allow users to add spacing between the rows in the month table.@majapw should we also add
horizontalSpacing
while we're at it? I don't currently have a use for it, but seems odd to only add one of the two. Maybe I should just add atableBorderSpacing
prop with two values:vertical
andhorizontal
?NB: I also added
showInputs
as a declared prop for the wrapper. This was previously throwing a linting error.