[vertical scrollable] add spacing and scrollbar theme #1298

amhunt · 2018-08-04T00:10:13Z

This adds three new theme variables, primarily aimed at the vertical scrollable configuration.

Adds the option to remove the scrollbar from the vertical scrollable orientation. The scroll bar looks bad on the lux calendar and also pushes things over so that elements are no longer aligned.
Adds a horizontal spacing prop for months (i.e. the padding around each individual month)
Adds a horizontal spacing prop for the daypicker container (padding around all of the months).

I also rewrote the getCalendarMonthWidth function because it was using the wrong value (9 instead of 13 in the default case).

@majapw @ljharb

coveralls · 2018-08-04T00:18:31Z

Coverage remained the same at 84.82% when pulling 0ec2d72 on amh-add-theme-options into ffe96d9 on master.

ljharb · 2018-08-04T04:04:34Z

src/components/CalendarMonthGrid.jsx

@@ -260,6 +260,7 @@ class CalendarMonthGrid extends React.Component {
      isFocused,
      isRTL,
      styles,
+      theme,


theme: { reactDates: theme }?

ljharb · 2018-08-04T04:05:15Z

src/components/CalendarMonthGrid.jsx

-export default withStyles(({ reactDates: { color, zIndex } }) => ({
+export default withStyles(({
+  reactDates: {
+    color, noScrollBarOnVerticalScrollable, spacing, zIndex,


each of these needs to be on its own line, if the entire object literal can't fit on one line

majapw · 2018-08-07T00:31:38Z

Taking a look now!

majapw

This looks phenom @amhunt!

So I think that the calendarmonth padding should be a prop---but I think I'm making an assumption that it works a certain way when I say that. If you were to set that value to 0 what would it look like? What is the interplay between the month padding and picker padding?

majapw · 2018-08-07T00:39:28Z

src/components/CalendarMonthGrid.jsx

+export default withStyles(({
+  reactDates: {
+    color,
+    noScrollBarOnVerticalScrollable,


QQ: can you investigate as to whether the scrollbar is an issue on the VERTICAL_ORIENTATION as well? It may make sense to have noScrollBarOnVertical be the option

It isn't/if there were an issue, it could be managed outside of the vertical calendar (because the largest div for the vertical calendar is the outermost div, whereas the smaller surrounding div in VERTICAL_SCROLLABLE is the one that's causing the scrollbar to appear).

majapw · 2018-08-07T00:53:05Z

src/components/CalendarMonthGrid.jsx

-    const calendarMonthWidth = getCalendarMonthWidth(daySize);
+    const calendarMonthWidth = getCalendarMonthWidth(
+      daySize,
+      theme.spacing.calendarMonthHorizontalPadding,


This is the reason why calendarMonthHorizontalPadding seems like it could be a prop instead of a theme variable. I get that it's only related to CSS---but because it also influences the animations and open source consumers have asked for this as well, I would maybe make this one a prop. We could call it horizontalMonthPadding to align with verticalSpacing and verticalHeight props.

majapw · 2018-08-07T00:53:39Z

src/components/CalendarMonthGrid.jsx

@@ -376,7 +387,7 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({

  CalendarMonthGrid__horizontal: {
    position: 'absolute',
-    left: 9,
+    left: spacing.dayPickerHorizontalPadding,


I think this makes sense in the theme.

majapw · 2018-08-07T00:55:20Z

src/components/DayPicker.jsx

@@ -956,7 +962,8 @@ class DayPicker extends React.Component {
      : 0;

    const firstVisibleMonthIndex = this.getFirstVisibleIndex();
-    const wrapperHorizontalWidth = (calendarMonthWidth * numberOfMonths) + (2 * DAY_PICKER_PADDING);
+    const wrapperHorizontalWidth = (calendarMonthWidth * numberOfMonths)
+      + (2 * dayPickerHorizontalPadding);


What do we use this for?

This is the width of the calendar for horizontal orientation. So if the outside padding of the dayPicker is 9 and the calendarMonth padding is 13, then the width of the horizontal calendar would be: 2*9 + 2*((2*13) + width_of_calendar_without_padding). Does that answer your question?

majapw · 2018-08-07T00:55:40Z

src/components/DayPicker.jsx

+
+export default withStyles(({
+  reactDates: {
+    color, font, noScrollBarOnVerticalScrollable, spacing, zIndex,


nit: I'd prefer these to be on their own lines (for ease of rebasing in the future)

+1, see #1298 (comment)

majapw · 2018-08-07T00:56:16Z

src/utils/getCalendarMonthWidth.js

-export default function getCalendarMonthWidth(daySize) {
-  return (7 * (daySize + 1)) + (2 * (CALENDAR_MONTH_PADDING + 1));
+export default function getCalendarMonthWidth(daySize, calendarMonthPadding) {
+  return (7 * daySize) + (2 * calendarMonthPadding) + 1;


This makes way more sense :P

amhunt · 2018-08-08T04:34:53Z

@majapw updated :) I made the horizontalMonthPadding a prop. I also removed three styles that didn't have corresponding style objects in CalendarMonth... idk why they were there.

My one remaining question - how should I handle the default case? I just set it as default 13 at all levels - or should it be default 13 at the top level and then a required prop below?

ljharb · 2018-08-08T04:38:13Z

src/components/CalendarMonthGrid.jsx

@@ -386,6 +400,13 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({
  CalendarMonthGrid__vertical_scrollable: {
    margin: '0 auto',
    overflowY: 'scroll',
+    ...noScrollBarOnVerticalScrollable && {


could we add parens here to make it clear that the entire thing is spread, not just the first conditional?

ljharb · 2018-08-08T04:38:43Z

src/components/DayPicker.jsx

@@ -260,7 +264,8 @@ class DayPicker extends React.Component {

    if (nextProps.daySize !== daySize) {
      this.setState({
-        calendarMonthWidth: getCalendarMonthWidth(nextProps.daySize),
+        calendarMonthWidth:
+          getCalendarMonthWidth(nextProps.daySize, horizontalMonthPadding),


this should be instead formatted like this:

calendarMonthWidth: getCalendarMonthWidth( nextProps.daySize, horizontalMonthPadding, ),

ljharb · 2018-08-08T04:39:05Z

src/components/DayPicker.jsx

@@ -1210,5 +1231,12 @@ export default withStyles(({ reactDates: { color, font, zIndex } }) => ({
    right: 0,
    left: 0,
    overflowY: 'scroll',
+    ...noScrollBarOnVerticalScrollable && {


also parens here?

majapw

Love it! :) Thank you so much @amhunt for being patient!

majapw · 2018-08-10T16:32:00Z

src/components/CalendarMonth.jsx

@@ -187,9 +189,7 @@ class CalendarMonth extends React.Component {
      <div
        {...css(
          styles.CalendarMonth,
-          orientation === HORIZONTAL_ORIENTATION && styles.CalendarMonth__horizontal,
-          orientation === VERTICAL_ORIENTATION && styles.CalendarMonth__vertical,
-          verticalScrollable && styles.CalendarMonth__verticalScrollable,


Hey @amhunt Did these get intentionally removed?

Ah I see below that they don't actually exist :x

amhunt force-pushed the amh-add-theme-options branch from a6b7134 to 5cfcda4 Compare August 4, 2018 00:10

ljharb reviewed Aug 4, 2018

View reviewed changes

amhunt force-pushed the amh-add-theme-options branch 2 times, most recently from 64ac9b1 to 2242529 Compare August 6, 2018 21:31

[verticalscrollable] add spacing and scrollbar theme

bc4cae4

amhunt force-pushed the amh-add-theme-options branch from 2242529 to bc4cae4 Compare August 6, 2018 21:41

majapw reviewed Aug 7, 2018

View reviewed changes

amhunt force-pushed the amh-add-theme-options branch from 1a98b60 to d775e60 Compare August 8, 2018 04:29

ljharb reviewed Aug 8, 2018

View reviewed changes

amhunt force-pushed the amh-add-theme-options branch from d775e60 to 1e674d0 Compare August 8, 2018 04:45

implement maja suggestions

0ec2d72

amhunt force-pushed the amh-add-theme-options branch from 1e674d0 to 0ec2d72 Compare August 8, 2018 18:18

majapw approved these changes Aug 10, 2018

View reviewed changes

majapw merged commit dcb346e into master Aug 10, 2018

majapw deleted the amh-add-theme-options branch August 10, 2018 16:53

majapw mentioned this pull request Sep 13, 2018

Date Picker width not being calculated #1352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vertical scrollable] add spacing and scrollbar theme #1298

[vertical scrollable] add spacing and scrollbar theme #1298

amhunt commented Aug 4, 2018

coveralls commented Aug 4, 2018 •

edited

ljharb Aug 4, 2018

ljharb Aug 4, 2018

majapw commented Aug 7, 2018

majapw left a comment

majapw Aug 7, 2018

amhunt Aug 7, 2018

majapw Aug 7, 2018

majapw Aug 7, 2018

majapw Aug 7, 2018

amhunt Aug 7, 2018

majapw Aug 7, 2018

ljharb Aug 7, 2018

majapw Aug 7, 2018

amhunt commented Aug 8, 2018

ljharb Aug 8, 2018

ljharb Aug 8, 2018

ljharb Aug 8, 2018

majapw left a comment

majapw Aug 10, 2018

majapw Aug 10, 2018

[vertical scrollable] add spacing and scrollbar theme #1298

[vertical scrollable] add spacing and scrollbar theme #1298

Conversation

amhunt commented Aug 4, 2018

coveralls commented Aug 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw commented Aug 7, 2018

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amhunt commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 4, 2018 •

edited