DaySize update #406

moonboots · 2017-03-27T21:07:10Z

Continuation of Maja's original PR to add customizable daySize's. I rebased and inlined a 39 + 1 calculation to 40 daySize default for a slightly cleaner number.

I punted on my request to make the padding configurable as well. It's on of a few remaining hardcoded sizes, and I think we can revisit with a more general solution like sizing with pure css. On this topic, is there any sizing logic that strictly requires js? I think the transition animation should be doable without knowing pixel sizes in js, e.g. using translate -100%.

This current version should be sufficient for our use, so PTAL.

coveralls · 2017-03-27T21:25:51Z

Coverage increased (+0.1%) to 87.437% when pulling 6dfdab4 on moonboots:maja-add-calendarday-size-prop into b90ee78 on airbnb:master.

coveralls · 2017-03-27T21:50:26Z

Coverage increased (+0.08%) to 87.406% when pulling e218f00 on moonboots:maja-add-calendarday-size-prop into b90ee78 on airbnb:master.

majapw

Seems reasonable, but maybe we should create a constants file since we have a magic number all over the place rn.

Also things don't quite seem aligned. Can you investigate the right padding on this screenshot?

majapw · 2017-03-29T21:27:26Z

src/components/CalendarDay.jsx

@@ -17,6 +18,7 @@ const propTypes = forbidExtraProps({

 const defaultProps = {
  day: moment(),
+  daySize: 40,


Should we start like a theme constants file? We're going to grow the random numbers that we have and this default gets used throughout the tree.

moonboots · 2017-03-30T18:59:19Z

Thanks @majapw, extracted DAY_SIZE into a constant fixed the right padding calc

coveralls · 2017-03-30T19:12:05Z

Coverage increased (+0.1%) to 87.421% when pulling 2b1d9ff on moonboots:maja-add-calendarday-size-prop into f1cee85 on airbnb:master.

coveralls · 2017-03-30T19:12:24Z

Coverage increased (+0.1%) to 87.421% when pulling 2b1d9ff on moonboots:maja-add-calendarday-size-prop into f1cee85 on airbnb:master.

ljharb

Could this potentially be semver-major because of the css changes?

ljharb · 2017-03-30T19:29:03Z

src/components/CalendarMonth.jsx

@@ -25,6 +26,7 @@ const propTypes = forbidExtraProps({
  enableOutsideDays: PropTypes.bool,
  modifiers: PropTypes.object,
  orientation: ScrollableOrientationShape,
+  daySize: PropTypes.number,


Should this allow any number, or only a positive integer? If the latter, let's use airbnb-prop-types to restrict it.

i concur. airbnb-prop-types is already available to you.

ljharb · 2017-03-30T19:29:30Z

src/components/CalendarMonthGrid.jsx

@@ -33,6 +35,7 @@ const propTypes = forbidExtraProps({
  onMonthTransitionEnd: PropTypes.func,
  renderDay: PropTypes.func,
  transformValue: PropTypes.string,
+  daySize: PropTypes.number,


Since this propType definition appears in multiple places, let's use a shared one so the source of truth only lives in one place.

Isn't that true of like... 90% of react-dates proptypes though?

yes, and ideally they'd be imported all from one place too - my personal preference is to export it from the leaf-est component that needs it.

I think that is clean up that we could do all at once in a follow up PR.

ljharb · 2017-03-30T19:29:43Z

src/components/DayPicker.jsx

@@ -38,6 +39,7 @@ const propTypes = forbidExtraProps({
  hidden: PropTypes.bool,
  initialVisibleMonth: PropTypes.func,
  renderCalendarInfo: PropTypes.func,
+  daySize: PropTypes.number,


ljharb · 2017-03-30T19:29:52Z

src/components/DayPickerRangeController.jsx

@@ -43,6 +44,7 @@ const propTypes = forbidExtraProps({
  orientation: ScrollableOrientationShape,
  withPortal: PropTypes.bool,
  initialVisibleMonth: PropTypes.func,
+  daySize: PropTypes.number,


ljharb · 2017-03-30T19:29:59Z

src/shapes/SingleDatePickerShape.js

@@ -35,10 +35,12 @@ export default {
  keepOpenOnDateSelect: PropTypes.bool,
  reopenPickerOnClearDate: PropTypes.bool,
  renderCalendarInfo: PropTypes.func,
+  daySize: PropTypes.number,


moonboots · 2017-03-31T19:54:44Z

@majapw @ljharb Updated with nonNegativeNumber, deferred the proptype crunch, ptal

majapw

Did you ever look into the weird padding issue?

majapw · 2017-03-31T20:32:58Z

constants.js

@@ -11,4 +11,6 @@ module.exports = {

  ANCHOR_LEFT: 'left',
  ANCHOR_RIGHT: 'right',
+
+  DAY_SIZE: 39,


Didn't you change this one to 40?

40 was a mistake when I misinterpreted the getCalendarMonthWidth logic. It was causing the missing right padding you pointed out.

majapw · 2017-03-31T20:33:43Z

src/components/CalendarMonth.jsx

@@ -25,6 +26,7 @@ const propTypes = forbidExtraProps({
  enableOutsideDays: PropTypes.bool,
  modifiers: PropTypes.object,
  orientation: ScrollableOrientationShape,
+  daySize: nonNegativeInteger,


Can you double-check that this doesn't need to be nonNegativeInteger()?

Tested with daySize={-1}, nonNegativeInteger without extra function invocation correctly prints to console.error.

Oh! Great. :)

thats an omission in airbnb-prop-types, which i'll fix in the next semver-major :-)

@majapw @ljharb I think this does need to be nonNegativeInteger(). This leads to a lot of warnings for me, of the form:

Warning: CalendarDay: type specification of prop `daySize` is invalid; the type checker function must return `null` or an `Error` but returned a function. You may have forgotten to pass an argument to the type checker creator (arrayOf, instanceOf, objectOf, oneOf, oneOfType, and shape all require an argument).

I have opened a PR to fix this issue: #664

in fact nonNegativeInteger isn't thunkified, so this is correct as-is https://unpkg.com/airbnb-prop-types@2.8.0/src/nonNegativeInteger.js

@palashkaria what version of react are you using?

ljharb

My concerns have been addressed

ljharb · 2017-03-31T20:52:43Z

src/components/CalendarMonth.jsx

@@ -25,6 +26,7 @@ const propTypes = forbidExtraProps({
  enableOutsideDays: PropTypes.bool,
  modifiers: PropTypes.object,
  orientation: ScrollableOrientationShape,
+  daySize: nonNegativeInteger,


thats an omission in airbnb-prop-types, which i'll fix in the next semver-major :-)

moonboots force-pushed the maja-add-calendarday-size-prop branch from 6dfdab4 to e218f00 Compare March 27, 2017 21:34

majapw requested changes Mar 30, 2017

View reviewed changes

majapw mentioned this pull request Mar 30, 2017

Adds daySize prop to the DRP/SDP/DP that scales everything accordingly #249

Closed

moonboots force-pushed the maja-add-calendarday-size-prop branch 2 times, most recently from 47b5eb3 to 2b1d9ff Compare March 30, 2017 18:58

ljharb requested changes Mar 30, 2017

View reviewed changes

moonboots force-pushed the maja-add-calendarday-size-prop branch from 2b1d9ff to 553e126 Compare March 31, 2017 20:15

majapw requested changes Mar 31, 2017

View reviewed changes

majapw approved these changes Mar 31, 2017

View reviewed changes

ljharb approved these changes Mar 31, 2017

View reviewed changes

moonboots force-pushed the maja-add-calendarday-size-prop branch 2 times, most recently from fc8272c to cbe1460 Compare March 31, 2017 21:51

majapw force-pushed the maja-add-calendarday-size-prop branch 4 times, most recently from f92b63c to b5bbab7 Compare March 31, 2017 22:53

Adds daySize prop to the DRP/SDP/DP that scales everything accordingly

7c51f65

majapw force-pushed the maja-add-calendarday-size-prop branch from b5bbab7 to 7c51f65 Compare March 31, 2017 23:09

majapw merged commit 8b6e41c into react-dates:master Mar 31, 2017

palashkaria mentioned this pull request Aug 5, 2017

updating propType usage to nonNegativeInteger() #664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DaySize update #406

DaySize update #406

moonboots commented Mar 27, 2017

coveralls commented Mar 27, 2017

coveralls commented Mar 27, 2017

majapw left a comment

majapw Mar 29, 2017

moonboots commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

ljharb left a comment

ljharb Mar 30, 2017

majapw Mar 30, 2017

ljharb Mar 30, 2017

majapw Mar 30, 2017

ljharb Mar 31, 2017

majapw Mar 31, 2017 •

edited

ljharb Mar 30, 2017

ljharb Mar 30, 2017

ljharb Mar 30, 2017

moonboots commented Mar 31, 2017

majapw left a comment

majapw Mar 31, 2017

moonboots Mar 31, 2017

majapw Mar 31, 2017

majapw Mar 31, 2017

moonboots Mar 31, 2017

majapw Mar 31, 2017

ljharb Mar 31, 2017

palashkaria Aug 5, 2017 •

edited

ljharb Aug 6, 2017

ljharb Aug 6, 2017

ljharb left a comment

ljharb Mar 31, 2017

DaySize update #406

DaySize update #406

Conversation

moonboots commented Mar 27, 2017

coveralls commented Mar 27, 2017

coveralls commented Mar 27, 2017

majapw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moonboots commented Mar 30, 2017

coveralls commented Mar 30, 2017

coveralls commented Mar 30, 2017

ljharb 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

majapw Mar 31, 2017 • edited

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

moonboots commented Mar 31, 2017

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

palashkaria Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majapw Mar 31, 2017 •

edited

palashkaria Aug 5, 2017 •

edited