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.

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

make nav buttons optional #1160

Merged
merged 3 commits into from
May 14, 2018
Merged

make nav buttons optional #1160

merged 3 commits into from
May 14, 2018

Conversation

amhunt
Copy link
Contributor

@amhunt amhunt commented May 7, 2018

Summary

Beyond would like to be able to render our own nav buttons completely (rather than just be able to render inside of the existing nav button boxes). I believe that we can do this completely outside of the component by just adding a button that changes the number of months passed to DayPickerRangeController. However, to do this, we do not want to render the existing react-dates nav buttons. This adds a boolean prop that makes the nav buttons optional.

This should also be helpful if people want to display only a couple of months and not allow users to move infinitely into the future (or any other custom navigation features).

Reviewers

@majapw
cc: @ljharb

Storybook example of vertical_scrollable orientation (didn't exist before)

screen shot 2018-05-07 at 2 08 40 pm

Storybook example of new noNavButtons prop

screen shot 2018-05-07 at 2 53 12 pm

@coveralls
Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage increased (+0.02%) to 84.868% when pulling e6c7872 on amh-make-nav-optional into 4e2db9d on master.

@ljharb ljharb added the semver-minor: new stuff Any feature or API addition. label May 7, 2018
@ljharb ljharb requested a review from majapw May 7, 2018 22:27
@ljharb
Copy link
Member

ljharb commented May 7, 2018

This should also be helpful if people want to display only a couple of months and not allow users to move infinitely into the future (or any other custom navigation features).

I think isOutsideRange already covers that by disabling navigation to those months.

@majapw
Copy link
Collaborator

majapw commented May 8, 2018

I think isOutsideRange already covers that by disabling navigation to those months.

I'm working on this! Today hopefully. :)

I'll take a look at this PR today!

@amhunt amhunt force-pushed the amh-make-nav-optional branch from 8c812f2 to 785c5a1 Compare May 8, 2018 23:00
@amhunt amhunt force-pushed the amh-make-nav-optional branch from 785c5a1 to 2ee37da Compare May 8, 2018 23:01
@amhunt
Copy link
Contributor Author

amhunt commented May 8, 2018

@majapw do you mean this new prop will be useless once that feature is added? Or just that that one use case will be solved by your work? I think that this prop could still be useful, so that users don't have to create their own isOutsideOfRange function and update it every time they manually update the number of months or the starting month.

@amhunt amhunt force-pushed the amh-make-nav-optional branch from 2ee37da to afd3e36 Compare May 8, 2018 23:20
@amhunt
Copy link
Contributor Author

amhunt commented May 9, 2018

Also, @majapw, any idea what the travis failures are? Seems to be related to Node 6... but I have no idea why that would be the case or how the error message relates to this PR

@ljharb
Copy link
Member

ljharb commented May 10, 2018

@amhunt it's an npm 6 bug in node 6; it should be fixed now (by bypassing the broken part with an env var in our travis-ci). i'll rerun your errored builds.

@amhunt
Copy link
Contributor Author

amhunt commented May 11, 2018

@majapw ptal :)

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me.

@majapw majapw merged commit dce2fc4 into master May 14, 2018
@majapw majapw deleted the amh-make-nav-optional branch May 14, 2018 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor: new stuff Any feature or API addition.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants