Add support for positioning month navigation under the calendar #1784
Conversation
stories/DayPickerRangeController.js
Outdated
| /> | ||
| ))) | ||
| .add('with current week range selection', withInfo()(() => ( | ||
| <DayPickerRangeControllerWrapper | ||
| onOutsideClick={action('DayPickerRangeController::onOutsideClick')} | ||
| onPrevMonthClick={action('DayPickerRangeController::onPrevMonthClick')} | ||
| onNextMonthClick={action('DayPickerRangeController::onNextMonthClick')} | ||
| startDateOffset={day => day.startOf('week')} | ||
| endDateOffset={day => day.endOf('week')} | ||
| startDateOffset={(day) => day.startOf('week')} |
There was a problem hiding this comment.
this repo hasn't upgraded to the latest styleguide yet, so i believe this change will cause the linter to fail. is your editor set up to eslint autofix, as opposed to (for example) directly running prettier?
There was a problem hiding this comment.
@ljharb yep, it's set up to autofix 🤦♀ I noticed the CI failed immediately on several lint errors
There was a problem hiding this comment.
odd; if it's using a local eslint then it shouldn't produce this change (but using a global one, it might)
either way, easy fix :-)
.vscode/settings.json
Outdated
| @@ -0,0 +1,2 @@ | |||
| { | |||
| } | |||
|
@ljharb added more variations to components that already had one for custom navigation, let me know if you think I should add others! |
| @@ -219,6 +230,12 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({ | |||
| zIndex: zIndex + 2, | |||
| }, | |||
|
|
|||
| DayPickerNavigation__bottom: { | |||
There was a problem hiding this comment.
@ljharb I tried to avoid adding any styles that implicate layout to DayPickerNavigation for custom navigation, but this was the best I could think to come up with since any absolutely positioned children will be taken out of the flow—and then there won't be enough space under the calendar. any thoughts on this?
|
@caseklim sorry i wasn't clear; stories in this repo aren't tested variations, so they don't actually gate any merges afaik (the karma jobs are often flaky). we'd need actual unit tests to be able to reliably prevent regressions. More stories are great tho! |
| @@ -219,6 +230,12 @@ export default withStyles(({ reactDates: { color, zIndex } }) => ({ | |||
| zIndex: zIndex + 2, | |||
| }, | |||
|
|
|||
| DayPickerNavigation__bottom: { | |||
| display: 'flex', | |||
There was a problem hiding this comment.
hmm - adding flexbox is a breaking change; it's not currently used anywhere in this repo, and react-dates has wider browser support than airbnb itself. is this something that could be overridden in an internal theme, and a different default selected?
There was a problem hiding this comment.
it's only for this use case of bottom navigation, so it won't be added to any other uses. I tried the non-flexbox workaround and couldn't get it working 😭 do we have any instances of using a theme in this repo other than a bunch of props? I could add dayPickerNavigationStyles or something like that...
There was a problem hiding this comment.
no, but airbnb uses its own theme and doesn't use this default one, afaik, so as long as there's any kind of workaround, that could go here and airbnb could use the flex styling.
i suppose since it's opt in, it wouldn't actually be a breaking change, but it would likely trigger bug reports that we'd have no way to fix :-/
There was a problem hiding this comment.
hmm it doesn't seem like this is something that would fit into a theme?
https://github.com/airbnb/react-dates/blob/master/src/theme/DefaultTheme.js
what do you think about dayPickerNavigationStyles? I was trying to avoid going through the hassle of adding another new prop but that's the only reasonable thing I can think to make it work in the default and non-default cases...
There was a problem hiding this comment.
went ahead and added that—let me know what you think :)
|
@ljharb ahhh didn't see the unit tests before! will add to that |
caseklim
force-pushed
the
bottom-nav-position
branch
from
13fec10 to
5021db8
Compare
| styles.DayPickerNavigation__bottom, | ||
| isDefaultNav && styles.DayPickerNavigation__bottomDefault, | ||
| ] : []), | ||
| hasCustomStyles && customStyles, |
There was a problem hiding this comment.
:-/ this allows full customization of all CSS, which is typically something we've avoided allowing.
cc @majapw @noratarano; do you think there's a way we could solve this with theming, or find a non-flex fallback?
There was a problem hiding this comment.
ahhh I see 😢 @majapw @noratarano happy to sit down with you in person on Thursday if that's easier to walk through the issue
There was a problem hiding this comment.
@ljharb I chatted with @noratarano and we feel that using inline styles (renamed to inlineStyles vs. customStyles) is the best option here given the implementation and timeline constraints. wdyt of leaving this as-is? if you're okay with this I noticed that coverage dropped, I'll make sure to up that percentage by adding more tests.
There was a problem hiding this comment.
It’s certainly not my call, but i think allowing unrestricted styling is a major philosophical departure from both external (and to a lesser extent, internal) policies thus far.
caseklim
force-pushed
the
bottom-nav-position
branch
2 times, most recently
from
b49abe0 to
a1bf0c4
Compare
caseklim
force-pushed
the
bottom-nav-position
branch
from
a1bf0c4 to
5a769af
Compare
caseklim
force-pushed
the
bottom-nav-position
branch
from
5a769af to
c82f9b0
Compare
Summary
Add a variation where the next and previous month navigation links can be positioned below the calendar months.
Screenshots
Default navigation
Custom navigation