Skip to content

Conversation

@theolampert
Copy link

I'm unsure if this is actually wanted or not but there seems to be a few others wanting to use DateRangePicker component with vertical scrolling.

Currently it works fine but results in an ugly proptype warning. This PR attempts to address that by importing ScrollableOrientationShape rather than OrientationShape within DateRangePickerShape

@coveralls
Copy link

Coverage Status

Coverage remained the same at 84.875% when pulling 8e640c2 on theolampert:master into 0e019f5 on airbnb:master.

@ljharb ljharb reopened this Dec 12, 2019
@ljharb ljharb added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Dec 12, 2019
@ljharb
Copy link
Member

ljharb commented Dec 12, 2019

Sorry for the delay. This is a great bug fix, but it needs a regression test, can you add one?

@theolampert
Copy link
Author

@ljharb sorry closed this thinking it was no longer needed. Yeah I can add a test.

@theolampert
Copy link
Author

@ljharb Could you take another look?

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.

LGTM pending a rebase

@theolampert
Copy link
Author

@ljharb done 🙂

@ljharb ljharb added semver-minor: new stuff Any feature or API addition. and removed semver-patch: fixes/refactors/etc Anything that's not major or minor. labels Nov 30, 2021
@ljharb
Copy link
Member

ljharb commented Nov 30, 2021

@theolampert I've rebased this; could we add some storybook stories that cover this new orientation?

@codecov
Copy link

codecov bot commented Nov 30, 2021

Codecov Report

Merging #1353 (a495c2b) into master (9494adb) will increase coverage by 0.16%.
The diff coverage is n/a.

❗ Current head a495c2b differs from pull request most recent head 87fa821. Consider uploading reports for the commit 87fa821 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1353      +/-   ##
==========================================
+ Coverage   80.02%   80.19%   +0.16%     
==========================================
  Files          57       57              
  Lines        2303     2292      -11     
  Branches      636      636              
==========================================
- Hits         1843     1838       -5     
+ Misses        271      267       -4     
+ Partials      189      187       -2     
Impacted Files Coverage Δ
src/components/DateRangePickerInputController.jsx 94.91% <0.00%> (-0.17%) ⬇️
src/components/DayPickerSingleDateController.jsx 93.22% <0.00%> (-0.07%) ⬇️
src/components/DayPickerRangeController.jsx 92.10% <0.00%> (-0.04%) ⬇️
src/utils/isDayVisible.js 100.00% <0.00%> (ø)
src/components/DateInput.jsx 72.85% <0.00%> (ø)
src/components/DayPicker.jsx 72.11% <0.00%> (ø)
src/components/CalendarDay.jsx 61.01% <0.00%> (ø)
src/components/SingleDatePicker.jsx 64.58% <0.00%> (ø)
src/components/DayPickerNavigation.jsx 84.61% <0.00%> (ø)
src/components/KeyboardShortcutRow.jsx 75.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9494adb...87fa821. Read the comment docs.

@theolampert
Copy link
Author

@ljharb I wasn't able to get storybook to build on an M1 mac, complains about node-sass missing so I assume it's some native module thing I didn't really want to debug. I'll try and take a look from an intel machine either tonight or tomorrow.

@ljharb
Copy link
Member

ljharb commented Nov 30, 2021

@theolampert node-sass requires very specific node versions - in this case, i think node 14? and on an M1 mac, node < 16 requires installing it via rosetta.

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.

3 participants