Add initialVisibleMonth fn prop to DayPicker by travisbloom · Pull Request #70 · react-dates/react-dates

travisbloom · 2016-09-09T18:17:10Z

Adds a new prop to DateRangePicker, SingleDatePicker, and DayPicker
that passes a fn to DayPicker. On the initial focus, that fn will be called
and set the initial month to the returned moment object.

solves issue #17

travisbloom · 2016-09-09T18:18:19Z

test/components/DayPicker_spec.jsx

      });
    });

+    describe('#componentWillReceiveProps', () => {


This test can't run currently because of https://github.com/airbnb/react-dates/blob/master/test/components/DayPicker_spec.jsx#L225

Is there an open issue/discussion for how to solve? getting It looks like you calledmount()without a global document being loaded. when i remove skip

Yeah, at Airbnb we use jsdom afaict and mocha-wrap (https://github.com/ljharb/mocha-wrap) plugin, but I think there was some discussion on what we plan to do for our open-source repos. @ljharb do you have thoughts on getting enzym's mount to work in this repo? I haven't really thought about it, but we should really open an issue I guess.

In order to do that, we'd need to run the test suite twice - once with jsdom, and once without it - and be able to mark tests that require the DOM such that they'd be skipped when running without jsdom.

I don't yet have a clean solution for this; i think it's OK to skip those tests for now.

Yeah let's skip them.

travisbloom · 2016-09-09T19:24:16Z

@ljharb fixes added, let me know if you have more insight into the It looks like you called mount() without a global document being loaded issue

travisbloom · 2016-09-09T19:50:39Z

also the linter is failing because of https://github.com/airbnb/react-dates/pull/70/files#diff-883056b4b11263241c339f927e93411bR75

the issue is that if I make it one line I get exceeds max length of 100 lint error.

Is the expected style below?

const triggerInitialVisibleMonth = (props) => 
  props.hidden ? moment() : 
  props.initialVisibleMonth();

ljharb · 2016-09-09T22:00:09Z

const triggerInitialVisibleMonth = ({ hidden, initialVisibleMonth }) => (
  hidden ? moment() : initialVisibleMonth()
);

travisbloom · 2016-09-12T12:56:48Z

@ljharb whoops, totally spaced on that, good call :)

travisbloom · 2016-09-13T21:06:39Z

Any reason the tests that are failing wouldn't be running/failing on my local machine?

ljharb · 2016-09-14T05:28:21Z

@travisbloom yes - rebase on latest master; one of the dependencies had to be locked down to avoid a bug.

majapw · 2016-09-15T18:28:26Z

src/components/DayPicker.jsx

 };

+const triggerInitialVisibleMonth = (props) => (
+  props.hidden ? moment() : props.initialVisibleMonth()


I'm not sure that we need this logic to be perfectly honest. The constructor is only called once and even if the datepicker is visible at that point, it will be the first time its ever rendered so I think we can just set state.currentMonth to props.initialVisibleMonth(). What is the edge case this is trying to solve?

Can we pull this as an instance method onto the class, named as getInitialVisibleMonth or something? That way we just have access to the props straight up.

Actually we don't even call this more than once. Why don't we just have this logic directly in the constructor?

@ljharb asked for it to be broken out in to a seperate function in case we need the logic elsewhere, not 100% where that would be.

@majapw let me know which you prefer, getting conflicting suggestions :)

Oh sorry about that! I think if it's only used once, we should not pull it out. :) Let's go with that.

Since it's used in two separate components, my hope was that we'd define it in one place and import it in both.

majapw · 2016-09-15T18:29:09Z

src/components/DayPicker.jsx

  constructor(props) {
    super(props);
+
+    this.hasSetInitialVisibleMonth = !props.hidden;


Similarly I don't think we need the hasSetInitialVisibleMonth logic either.

majapw · 2016-09-15T18:32:22Z

Hey @travisbloom, this is looking good, but I'm a bit confused about the constructor logic you've included.

travisbloom · 2016-09-16T12:49:19Z

@majapw my understanding was we only want initialVisibleMonth fn to trigger once, when the calendar opens initially. The constructor logic ensures that the fn will be called on mount, or if the component is not focused yet, it will call the first time the nextProps has focused === true...

Now that I think about it, it would prob be better to trigger it every time it opens...is that the expected behavior you were thinking?

travisbloom · 2016-09-16T12:49:53Z

if thats the case, I'd propose changing initialVisibleMonth to visibleMonthOnFocus

majapw · 2016-09-26T15:14:32Z

Hmm, let me review again with that in mind. I guess the initialVisibleMonth does make sense... I was just misunderstanding the implementation I am pretty sure.

We can maybe augment it in the future to do kind of a fully controlled visibleMonthOnFocus situation, but for now I think this is sufficient.

majapw

Hi @travisbloom, sorry for the long delay in getting this out the door. Let's leave the implementation as is, but address the small nits in this review. After that I'm happy to merge this in ASAP. :)

majapw · 2016-09-29T16:08:47Z

src/components/DateRangePicker.jsx

          onNextMonthClick={onNextMonthClick}
          monthFormat={monthFormat}
          withPortal={withPortal || withFullScreenPortal}
+          hidden={!focusedInput}


Can we change this name to isVisible instead of hidden? I think that is more in line with the other nomenclature in these files.

I think once we move to react-with-styles, we hopefully won't need this anyways. since the component will be unmounted/remounted every time.

:( unfortunately I had it named as visible before but @ljharb asked to change it to be hidden so we didn't have a boolean prop that defaulted to true.

Let me know what you would prefer now.

Man, I should really have read @ljharb's comments more carefully before. :) hidden sounds good to me

majapw · 2016-09-29T16:11:37Z

src/components/DayPicker.jsx

 };

+const triggerInitialVisibleMonth = (props) => (
+  props.hidden ? moment() : props.initialVisibleMonth()


Can we pull this as an instance method onto the class, named as getInitialVisibleMonth or something? That way we just have access to the props straight up.

Actually we don't even call this more than once. Why don't we just have this logic directly in the constructor?

majapw · 2016-09-29T16:19:05Z

src/components/SingleDatePicker.jsx

          onNextMonthClick={onNextMonthClick}
          monthFormat={monthFormat}
          withPortal={withPortal || withFullScreenPortal}
+          hidden={!focused}


same, let's name this as isVisible

majapw · 2016-09-29T16:20:38Z

We'll also need to rebase before getting this out! :D Thanks so much @travisbloom

travisbloom · 2016-10-08T17:43:03Z

@majapw @ljharb a lot of back and forth on what changes should be made :) rebased and made the requested fixes, let me know if thats inline with what we ended up wanting to do.

Adds a new prop to DateRangePicker, SingleDatePicker, and DayPicker that passes a fn to DayPicker. On the initial focus, that fn will be called and set the initial month to the returned moment object. solves issue react-dates#17

travisbloom · 2016-10-08T18:35:32Z

.istanbul.yml

  global:
    statements: 89
-    branches: 70
+    branches: 69


inline with #70 (comment), updating coverage requirements

coveralls · 2016-10-08T19:09:01Z

Coverage decreased (-0.3%) to 87.893% when pulling bff83ef on rocketrip:master into 8c03d77 on airbnb:master.

majapw · 2016-10-10T19:14:37Z

Great! Thank you for all of this @travisbloom.

travisbloom reviewed Sep 9, 2016
View reviewed changes

travisbloom force-pushed the master branch from c12e99f to 9cd6fa2 Compare September 9, 2016 19:23

travisbloom force-pushed the master branch from 9cd6fa2 to ff8b90f Compare September 12, 2016 12:56

travisbloom force-pushed the master branch from ff8b90f to ba4f921 Compare September 12, 2016 17:05

ljharb added the semver-minor: new stuff Any feature or API addition. label Sep 12, 2016

travisbloom force-pushed the master branch from ba4f921 to 9f553b2 Compare September 14, 2016 12:53

majapw reviewed Sep 15, 2016

View reviewed changes

ljharb mentioned this pull request Sep 16, 2016

Set date range #86

Closed

majapw added the needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. label Sep 26, 2016

majapw requested changes Sep 29, 2016

View reviewed changes

majapw mentioned this pull request Sep 30, 2016

Force minimum dates to affect the current month #103

Closed

travisbloom force-pushed the master branch 2 times, most recently from ed808c5 to 81e0c95 Compare October 8, 2016 17:40

Add initialVisibleMonth fn prop to DayPicker

bff83ef

Adds a new prop to DateRangePicker, SingleDatePicker, and DayPicker that passes a fn to DayPicker. On the initial focus, that fn will be called and set the initial month to the returned moment object. solves issue react-dates#17

travisbloom force-pushed the master branch from 81e0c95 to bff83ef Compare October 8, 2016 18:34

travisbloom commented Oct 8, 2016

View reviewed changes

majapw approved these changes Oct 10, 2016

View reviewed changes

majapw removed the needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. label Oct 10, 2016

majapw merged commit 7d2e0d4 into react-dates:master Oct 10, 2016

This was referenced Oct 12, 2016

Open SingleDatePicker at the current selected date #92

Closed

Cannot indicate which month is visible on open #17

Closed

Uh oh!

Conversation

travisbloom commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbloom commented Sep 9, 2016

Uh oh!

travisbloom commented Sep 9, 2016

Uh oh!

ljharb commented Sep 9, 2016

Uh oh!

travisbloom commented Sep 12, 2016

Uh oh!

travisbloom commented Sep 13, 2016

Uh oh!

ljharb commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majapw Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majapw commented Sep 15, 2016

Uh oh!

travisbloom commented Sep 16, 2016

Uh oh!

travisbloom commented Sep 16, 2016

Uh oh!

majapw commented Sep 26, 2016

Uh oh!

majapw left a comment

Choose a reason for hiding this comment

Uh oh!

majapw Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majapw Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majapw commented Sep 29, 2016

Uh oh!

travisbloom commented Oct 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 8, 2016

Uh oh!

majapw commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

travisbloom commented Sep 9, 2016 •

edited

Loading

majapw Sep 29, 2016 •

edited

Loading

majapw Sep 29, 2016 •

edited

Loading

majapw Sep 29, 2016 •

edited

Loading