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

Jump to bottom

Responsivizes the DayPicker container #83

Merged
merged 1 commit into from
Oct 14, 2016
Merged

Responsivizes the DayPicker container #83

merged 1 commit into from
Oct 14, 2016

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Sep 13, 2016 •
edited
Loading

This PR addresses #80.

This pulls in http://tether.io/ which is a sweet library (thanks for the rec, @lencioni!). Basically it pins the DayPicker component to its scrollable parents on the right side and updates this tether if you resize the window.

There are a few things left to do:

to: @lencioni @isaachinman

@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Sep 13, 2016
@isaachinman
Copy link
Contributor

Wow, that was way quicker than I expected! Are you handling those three remaining tasks yourself, or saying you need help?

@majapw
Copy link
Collaborator Author

majapw commented Sep 15, 2016

It'll take a bit for me to fully get to it, but either is good. This step #1 was a proof of concept. @isaachinman if you would be interested in continuing the work, we would def be happy to accept the PR. :) I think the OutsideClickHandler work and nub functionality might be a bit tricky.

@isaachinman
Copy link
Contributor

@majapw Trying to get your branch up and running so I can see how much work this will involve, but having CSS problems. No matter what npm script I run, I can't get storybook.css inside the public dir to be generated. Any advice?

@majapw
Copy link
Collaborator Author

majapw commented Sep 15, 2016

@isaachinman What do mean by generating the storybook.css file? That file is static and all it really does is set the font to sans-serif, change the box-sizing and add the background grid to the storybook. Is the css not loading for the datepicker itself? I'm assuming you've done the standard npm install and are running storybook using the npm script, yeah? Are you getting any errors?

@isaachinman
Copy link
Contributor

Ah, I misunderstood then. Yes I've run npm i as well as the build script. The picker seems to be initialising itself upward, and thus mostly not-visible. See screenshot below. I assumed this was a CSS positioning thing, but maybe not?

screen shot 2016-09-15 at 18 07 13

@majapw
Copy link
Collaborator Author

majapw commented Sep 15, 2016

Well it's certainly weird that the storybook.css file seems not to be loading at all, but the css for the datepicker itself seems to be loading (as indicated by the input styling). This is def an issue with the tether set-up itself WHICH I SWEAR TO GOD WAS WORKING WHEN I PUSHED THIS. :| Let me investigate as I can repro.

@majapw
Copy link
Collaborator Author

majapw commented Sep 15, 2016 •
edited
Loading

@isaachinman So in the setup:

    this.tether = new Tether({
      element: this.dayPickerContainer,
      target: ReactDOM.findDOMNode(this.input),
      attachment: 'top left',
      targetAttachment: 'bottom left',
      offset: '-20px 0',
      constraints: [{
        to: 'scrollParent',
        attachment: 'together',
        pin: ['right'],
      }],
    });

I think the to: 'scrollParent', is the relevant part and if we change that to window, we get much better behavior. HOWEVER, you don't get like the expected body padding and the like. So scrollParent would be ideal. Examining the <body> tag that would constitute the datepicker's scroll parent in this scenario, we see that it only has a height of 50px and so the tether is behaving in the way it would if the window was too short. ... of course, it shouldn't be behaving this way because the pinning should only be happening on the right side of the scroll parent. Also maybe more important, I swear that this was working before and idk what the H happened here. :|

EDIT: try this new update with the attachment: 'none'

@majapw majapw mentioned this pull request Sep 15, 2016
Merged
@thj-dk
Copy link

thj-dk commented Sep 27, 2016

@majapw any ideas how to solve the OutsideClickHandler related problem?

@majapw
Copy link
Collaborator Author

majapw commented Sep 28, 2016

Sorry I have been traveling and it has significantly impacted my work here.

@thj-dk my initial thought on the OutsideClickHandler issue would be just to wrap the input components and daypicker components separately and then when those are used with the tether the outside click logic is including in the pieces that are moved around in the dom individually.

@majapw majapw mentioned this pull request Oct 12, 2016
Merged
@majapw majapw force-pushed the maja-responsivize-datepicker branch 2 times, most recently from 598686a to bde5ded Compare October 13, 2016 18:32
@majapw
Copy link
Collaborator Author

majapw commented Oct 13, 2016 •
edited
Loading

Okay so I think this works!

I just put the OutsideClickHandler on the DayPicker div always to solve the close on outside click problem. This has the disadvantage that when you click on an date input, it calls setState on the focus state twice (once in the outside click handler and once in onFocus), but I think that is okay for our usecase.

Here are kind of the last bits to do (but they're small):

  • Update tests to look for the Tether and not the OutsideClickHandler
  • Change the edge the datepicker is pinned to based on anchor direction?
  • Double-check all the difference variations of the datepickers (including the portal versions)

And then we'll be good to go!

@majapw majapw force-pushed the maja-responsivize-datepicker branch from bde5ded to d377d64 Compare October 13, 2016 19:11
@majapw
Copy link
Collaborator Author

majapw commented Oct 13, 2016 •
edited
Loading

Cool! I think this is ready! :D Just needs to wait on #118 to be merged in.

@lencioni @ljharb @airbnb/webinfra can you take a look?

@majapw majapw changed the title [WIP] Responsivizes the DayPicker container Responsivizes the DayPicker container Oct 13, 2016
@majapw majapw force-pushed the maja-responsivize-datepicker branch 2 times, most recently from a5605c2 to bb6dc37 Compare October 13, 2016 19:23
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.988% when pulling bb6dc37 on maja-responsivize-datepicker into eb4bf33 on master.

@majapw majapw force-pushed the maja-responsivize-datepicker branch from bb6dc37 to c93d955 Compare October 13, 2016 21:56
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.988% when pulling c93d955 on maja-responsivize-datepicker into bc06856 on master.

ljharb
ljharb approved these changes Oct 14, 2016
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems 👍

@majapw majapw merged commit f22a577 into master Oct 14, 2016
@majapw majapw deleted the maja-responsivize-datepicker branch October 14, 2016 04:37
@tvanro tvanro mentioned this pull request Oct 17, 2016
Closed
@isaachinman isaachinman mentioned this pull request Jan 30, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@majapw @isaachinman @thj-dk @coveralls @ljharb