Remove invokation of responsivizePickerPosition from componentDidMount #183

Ikana · 2016-11-16T13:24:23Z

This addressed the issue #181 on the storyboard, but should the method responsivizePickerPosition be invoked from an other method?

coveralls · 2016-11-16T13:35:15Z

Coverage remained the same at 86.293% when pulling 2cd1555 on Ikana:fix-get-client-bounding-rect into 462c8bb on airbnb:master.

majapw · 2016-11-16T22:50:40Z

This is not the correct solution. Based on my comment #181 (comment), let's change the responsivizePickerPosition in both files to be:

const { anchorDirection, horizontalMargin, withPortal, withFullScreenPortal } = this.props;
    const { dayPickerContainerStyles } = this.state;

    const isAnchoredLeft = anchorDirection === ANCHOR_LEFT;

    if (!withPortal && !withFullScreenPortal) {
      const containerRect = this.dayPickerContainer.getBoundingClientRect();
      const currentOffset = dayPickerContainerStyles[anchorDirection] || 0;
      const containerEdge = isAnchoredLeft ? containerRect[ANCHOR_RIGHT] : containerRect[ANCHOR_LEFT];

      this.setState({
        dayPickerContainerStyles: getResponsiveContainerStyles(
          anchorDirection,
          currentOffset,
          containerEdge,
          horizontalMargin
        ),
      });
    }

ljharb · 2016-11-16T23:04:42Z

@Ikana please rebase on the command line so any merge commits are excised. Thanks!

Ikana · 2016-11-16T23:31:28Z

I think I broke something the tests pass on my local repo.

Ikana · 2016-11-17T00:27:35Z

I think it has to do with the recent update of react, the error is:

Error: Cannot find module 'react/lib/CSSPropertyOperations'

And on the blog post for 15.4.0 mentioned that the private apis did change

ljharb · 2016-11-17T00:29:47Z

React 15.4 did break things; #184 will fix it.

Ikana · 2016-11-17T00:33:01Z

so it wasn't me 😅

ljharb · 2016-11-17T00:42:07Z

Nope! rebase and you should be set.

majapw · 2016-11-17T00:47:58Z

src/components/SingleDatePicker.jsx

@@ -89,7 +89,6 @@ export default class SingleDatePicker extends React.Component {
  /* istanbul ignore next */
  componentDidMount() {
    window.addEventListener('resize', this.responsivizePickerPosition);
-    this.responsivizePickerPosition();


Can you make the same change as you did in DateRangePicker here?

majapw · 2016-11-17T00:48:20Z

Once you make the last change, can you squash all your commit into one and rebase on the latest master?

Ikana · 2016-11-17T00:58:17Z

Sure thing

coveralls · 2016-11-17T05:24:20Z

Coverage decreased (-0.1%) to 86.271% when pulling 8451fb3 on Ikana:fix-get-client-bounding-rect into c108674 on airbnb:master.

coveralls · 2016-11-17T05:56:43Z

Coverage decreased (-0.1%) to 86.271% when pulling 8451fb3 on Ikana:fix-get-client-bounding-rect into c108674 on airbnb:master.

coveralls · 2016-11-17T06:03:16Z

Coverage decreased (-0.1%) to 86.271% when pulling 8451fb3 on Ikana:fix-get-client-bounding-rect into c108674 on airbnb:master.

majapw · 2016-11-17T20:59:38Z

src/components/SingleDatePicker.jsx

    window.addEventListener('resize', this.responsivizePickerPosition);
-    this.responsivizePickerPosition();
+    if (!withPortal && !withFullScreenPortal) {


This will not work on window resize still. Can you please update this file in the same way you did the DateRangePicker (ie put this check inside of the responsivizePickerPosition function instead of here)

coveralls · 2016-11-17T23:38:32Z

Coverage decreased (-0.6%) to 85.759% when pulling d057fb3 on Ikana:fix-get-client-bounding-rect into c108674 on airbnb:master.

ljharb · 2016-11-17T23:59:42Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Change Log

+## v4.0.2


@Ikana please do a rebase on the latest master rather than clicking "update branch", which creates a merge commit.

i am a little new to open source collaboration, do you know of a good tutorial on that, i thought i did it.

Hi @Ikana! Basically in your terminal you can run the following steps:

git checkout master git pull airbnb master git checkout fix-get-client-bounding-rect git rebase master git push -f origin fix-get-client-bound-rect

If you don't have airbnb configured as a remote branch, you'll have to do:

git remote add airbnb [email protected]:airbnb/react-dates.git

majapw · 2016-11-18T01:14:57Z

src/components/DateRangePicker.jsx

    window.addEventListener('resize', this.responsivizePickerPosition);
-    this.responsivizePickerPosition();
+    if (!withPortal && !withFullScreenPortal) {


We don't need this check here because we have it on line 293.

…ullScreenPortal are set to true When either portal option is set, the DayPicker is not rendered at all when hidden. What's more, it is centered on the screen and as a result does not need to be moved based on its location relative to the edge of the screen.

…r method in the SDP

coveralls · 2016-11-18T19:39:56Z

Coverage decreased (-0.1%) to 86.159% when pulling 1ced2f8 on Ikana:fix-get-client-bounding-rect into 231342a on airbnb:master.

majapw · 2016-11-18T19:43:12Z

@ljharb this should be good to go, can you do a last pass?

Ikana · 2016-11-18T20:00:32Z

Hey @majapw thanks a lot for the patience and the help with git

coveralls · 2016-11-18T20:07:38Z

Coverage decreased (-0.1%) to 86.159% when pulling b6246c6 on Ikana:fix-get-client-bounding-rect into 231342a on airbnb:master.

majapw · 2016-11-18T20:08:46Z

No problem @Ikana! :D Thank you so much for the contribution. :)

ljharb

LGTM

ljharb · 2016-11-18T21:36:41Z

src/components/DateRangePicker.jsx

+      const containerRect = this.dayPickerContainer.getBoundingClientRect();
+      const currentOffset = dayPickerContainerStyles[anchorDirection] || 0;
+      const containerEdge =
+        isAnchoredLeft ? containerRect[ANCHOR_RIGHT] : containerRect[ANCHOR_LEFT];


can this line be moved to the previous one, so there's no linebreak after an assignment operator?

it's too long, and I preferred having the ternary on one line rather than splitting after the ?. Which do you think is better?

i always prefer a long line to arbitrary line breaks - ling length restrictions are always silly, and a poor proxy for readability.

ljharb · 2016-11-18T21:36:49Z

src/components/SingleDatePicker.jsx

+      const containerRect = this.dayPickerContainer.getBoundingClientRect();
+      const currentOffset = dayPickerContainerStyles[anchorDirection] || 0;
+      const containerEdge =
+        isAnchoredLeft ? containerRect[ANCHOR_RIGHT] : containerRect[ANCHOR_LEFT];


majapw reviewed Nov 17, 2016

View reviewed changes

ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Nov 17, 2016

majapw reviewed Nov 17, 2016

View reviewed changes

ljharb requested changes Nov 17, 2016

View reviewed changes

majapw reviewed Nov 18, 2016

View reviewed changes

Ikana and others added 2 commits November 18, 2016 14:33

Add withPortal/withFullScreenPortal check to the responsivizeDayPicke…

b6246c6

…r method in the SDP

ljharb approved these changes Nov 18, 2016

View reviewed changes

majapw merged commit 283beb6 into react-dates:master Nov 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove invokation of responsivizePickerPosition from componentDidMount #183

Remove invokation of responsivizePickerPosition from componentDidMount #183

Ikana commented Nov 16, 2016

coveralls commented Nov 16, 2016

majapw commented Nov 16, 2016 •

edited

Loading

ljharb commented Nov 16, 2016

Ikana commented Nov 16, 2016

Ikana commented Nov 17, 2016

ljharb commented Nov 17, 2016

Ikana commented Nov 17, 2016

ljharb commented Nov 17, 2016

majapw Nov 17, 2016

majapw commented Nov 17, 2016

Ikana commented Nov 17, 2016

coveralls commented Nov 17, 2016

coveralls commented Nov 17, 2016

coveralls commented Nov 17, 2016

majapw Nov 17, 2016

coveralls commented Nov 17, 2016

ljharb Nov 17, 2016

Ikana Nov 18, 2016

majapw Nov 18, 2016

majapw Nov 18, 2016

coveralls commented Nov 18, 2016

majapw commented Nov 18, 2016

Ikana commented Nov 18, 2016

coveralls commented Nov 18, 2016

majapw commented Nov 18, 2016

ljharb left a comment

ljharb Nov 18, 2016

majapw Nov 18, 2016

ljharb Nov 18, 2016

ljharb Nov 18, 2016

Remove invokation of responsivizePickerPosition from componentDidMount #183

Remove invokation of responsivizePickerPosition from componentDidMount #183

Conversation

Ikana commented Nov 16, 2016

coveralls commented Nov 16, 2016

majapw commented Nov 16, 2016 • edited Loading

ljharb commented Nov 16, 2016

Ikana commented Nov 16, 2016

Ikana commented Nov 17, 2016

ljharb commented Nov 17, 2016

Ikana commented Nov 17, 2016

ljharb commented Nov 17, 2016

Choose a reason for hiding this comment

majapw commented Nov 17, 2016

Ikana commented Nov 17, 2016

coveralls commented Nov 17, 2016

coveralls commented Nov 17, 2016

coveralls commented Nov 17, 2016

Choose a reason for hiding this comment

coveralls commented Nov 17, 2016

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

coveralls commented Nov 18, 2016

majapw commented Nov 18, 2016

Ikana commented Nov 18, 2016

coveralls commented Nov 18, 2016

majapw commented Nov 18, 2016

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

majapw commented Nov 16, 2016 •

edited

Loading