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.

Sign up for GitHub

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

[vertical scrollable] add spacing and scrollbar theme #1298

Merged
merged 2 commits into from
Aug 10, 2018
Merged

[vertical scrollable] add spacing and scrollbar theme #1298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc4cae4
[verticalscrollable] add spacing and scrollbar theme
Aug 3, 2018
0ec2d72
implement maja suggestions
Aug 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/CalendarMonth.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ export default withStyles(({ reactDates: { color, font, spacing } }) => ({
CalendarMonth: {
background: color.background,
textAlign: 'center',
padding: '0 13px',
padding: `0 ${spacing.calendarMonthHorizontalPadding}px`,
verticalAlign: 'top',
userSelect: 'none',
},
Expand Down
24 changes: 21 additions & 3 deletions src/components/CalendarMonthGrid.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ class CalendarMonthGrid extends React.Component {
isFocused,
isRTL,
styles,
theme: { reactDates: theme },
phrases,
dayAriaLabelFormat,
transitionDuration,
Expand All @@ -272,7 +273,10 @@ class CalendarMonthGrid extends React.Component {
const isVerticalScrollable = orientation === VERTICAL_SCROLLABLE;
const isHorizontal = orientation === HORIZONTAL_ORIENTATION;

const calendarMonthWidth = getCalendarMonthWidth(daySize);
const calendarMonthWidth = getCalendarMonthWidth(
daySize,
theme.spacing.calendarMonthHorizontalPadding,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);

const width = isVertical || isVerticalScrollable
? calendarMonthWidth
Expand Down Expand Up @@ -363,7 +367,14 @@ class CalendarMonthGrid extends React.Component {
CalendarMonthGrid.propTypes = propTypes;
CalendarMonthGrid.defaultProps = defaultProps;

export default withStyles(({ reactDates: { color, zIndex } }) => ({
export default withStyles(({
reactDates: {
color,
noScrollBarOnVerticalScrollable,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

spacing,
zIndex,
},
}) => ({
CalendarMonthGrid: {
background: color.background,
textAlign: 'left',
Expand All @@ -376,7 +387,7 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({

CalendarMonthGrid__horizontal: {
position: 'absolute',
left: 9,
left: spacing.dayPickerHorizontalPadding,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense in the theme.

},

CalendarMonthGrid__vertical: {
Expand All @@ -386,6 +397,13 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({
CalendarMonthGrid__vertical_scrollable: {
margin: '0 auto',
overflowY: 'scroll',
...noScrollBarOnVerticalScrollable && {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'-webkitOverflowScrolling': 'touch',
'::-webkit-scrollbar': {
'-webkit-appearance': 'none',
display: 'none',
},
},
},

CalendarMonthGrid_month__horizontal: {
Expand Down
35 changes: 27 additions & 8 deletions src/components/DayPicker.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import {
} from '../constants';

const MONTH_PADDING = 23;
const DAY_PICKER_PADDING = 9;
const PREV_TRANSITION = 'prev';
const NEXT_TRANSITION = 'next';
const MONTH_SELECTION_TRANSITION = 'month_selection';
Expand Down Expand Up @@ -173,8 +172,10 @@ class DayPicker extends React.Component {
focusedDate = props.getFirstFocusableDay(currentMonth);
}

const { reactDates: { spacing: { calendarMonthHorizontalPadding } } } = props.theme;

const translationValue = props.isRTL && this.isHorizontal()
? -getCalendarMonthWidth(props.daySize)
? -getCalendarMonthWidth(props.daySize, calendarMonthHorizontalPadding)
: 0;

this.hasSetInitialVisibleMonth = !props.hidden;
Expand All @@ -183,7 +184,7 @@ class DayPicker extends React.Component {
monthTransition: null,
translationValue,
scrollableMonthMultiple: 1,
calendarMonthWidth: getCalendarMonthWidth(props.daySize),
calendarMonthWidth: getCalendarMonthWidth(props.daySize, calendarMonthHorizontalPadding),
focusedDate: (!props.hidden || props.isFocused) ? focusedDate : null,
nextFocusedDate: null,
showKeyboardShortcuts: props.showKeyboardShortcuts,
Expand Down Expand Up @@ -240,6 +241,7 @@ class DayPicker extends React.Component {
showKeyboardShortcuts,
onBlur,
renderMonthText,
theme: { reactDates: { spacing: calendarMonthHorizontalPadding } },
} = nextProps;
const { currentMonth } = this.state;

Expand All @@ -260,7 +262,8 @@ class DayPicker extends React.Component {

if (nextProps.daySize !== daySize) {
this.setState({
calendarMonthWidth: getCalendarMonthWidth(nextProps.daySize),
calendarMonthWidth:
getCalendarMonthWidth(nextProps.daySize, calendarMonthHorizontalPadding),
});
}

Expand Down Expand Up @@ -899,6 +902,7 @@ class DayPicker extends React.Component {
isFocused,
isRTL,
styles,
theme,
phrases,
verticalHeight,
dayAriaLabelFormat,
Expand All @@ -907,6 +911,8 @@ class DayPicker extends React.Component {
verticalBorderSpacing,
} = this.props;

const { reactDates: { spacing: { dayPickerHorizontalPadding } } } = theme;

const isHorizontal = this.isHorizontal();

const numOfWeekHeaders = this.isVertical() ? 1 : numberOfMonths;
Expand Down Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we use this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

// Adding `1px` because of whitespace between 2 inline-block
const fullHorizontalWidth = wrapperHorizontalWidth + calendarInfoPanelWidth + 1;

Expand Down Expand Up @@ -1093,7 +1100,12 @@ DayPicker.propTypes = propTypes;
DayPicker.defaultProps = defaultProps;

export { DayPicker as PureDayPicker };
export default withStyles(({ reactDates: { color, font, zIndex } }) => ({

export default withStyles(({
reactDates: {
color, font, noScrollBarOnVerticalScrollable, spacing, zIndex,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, see #1298 (comment)

},
}) => ({
DayPicker: {
background: color.background,
position: 'relative',
Expand Down Expand Up @@ -1147,15 +1159,15 @@ export default withStyles(({ reactDates: { color, font, zIndex } }) => ({
},

DayPicker_weekHeaders__horizontal: {
marginLeft: 9,
marginLeft: spacing.dayPickerHorizontalPadding,
},

DayPicker_weekHeader: {
color: color.placeholderText,
position: 'absolute',
top: 62,
zIndex: zIndex + 2,
padding: '0 13px',
padding: `0 ${spacing.calendarMonthHorizontalPadding}px`,
textAlign: 'left',
},

Expand Down Expand Up @@ -1210,5 +1222,12 @@ export default withStyles(({ reactDates: { color, font, zIndex } }) => ({
right: 0,
left: 0,
overflowY: 'scroll',
...noScrollBarOnVerticalScrollable && {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also parens here?

'-webkitOverflowScrolling': 'touch',
'::-webkit-scrollbar': {
'-webkit-appearance': 'none',
display: 'none',
},
},
},
}))(DayPicker);
4 changes: 4 additions & 0 deletions src/theme/DefaultTheme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,8 @@ export default {
},

spacing: {
calendarMonthHorizontalPadding: 13,
dayPickerHorizontalPadding: 9,
captionPaddingTop: 22,
captionPaddingBottom: 37,
inputPadding: 0,
Expand All @@ -176,6 +178,8 @@ export default {
arrowWidth: 24,
},

noScrollBarOnVerticalScrollable: false,

font: {
size: 14,
captionSize: 18,
Expand Down
6 changes: 2 additions & 4 deletions src/utils/getCalendarMonthWidth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const CALENDAR_MONTH_PADDING = 9;

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes way more sense :P

}
2 changes: 1 addition & 1 deletion test/utils/getCalendarMonthWidth_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ import getCalendarMonthWidth from '../../src/utils/getCalendarMonthWidth';

describe('#getCalendarMonthWidth', () => {
it('correctly calculates width for default day size of 39', () => {
expect(getCalendarMonthWidth(39)).to.equal(300);
expect(getCalendarMonthWidth(39, 13)).to.equal(300);
});
});