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

Add missing props from weekdayElement and caption Element #842

Merged
merged 2 commits into from
Feb 21, 2019

Conversation

johnjesse
Copy link
Contributor

Changes:

I think the typings on captionElement and weekdayElement are wrong, and I think the documentation is wrong too. Looking at the default props that DayPicker receives, it's clear that the months weekdaysShort and weekdaysLong are passed through to those elements if they are specified on the DayPicker. This makes sense, how else will those elements be localized correctly. Therefore if I provide my own elements for the caption or weekday they'll need those props too.

They actually get them 👍 so the only thing that's missing is to update the types. which I've done

I'd also would've updated the docs, but the template advised against that. I will volunteer to update them though :).

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #842 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #842   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645

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 39ad48e...ecf6805. Read the comment docs.

2 similar comments
@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #842 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #842   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645

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 39ad48e...ecf6805. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #842 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #842   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645

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 39ad48e...ecf6805. Read the comment docs.

@giladgray
Copy link

in props.d.ts, months is typed as a 12-element tuple:

months?: [
string,
string,
string,
string,
string,
string,
string,
string,
string,
string,
string,
string
];

think it's worth duplicating that here? or, better, making a type alias MonthsArray for that gross tuple and reusing that.

@johnjesse
Copy link
Contributor Author

See #843 - depending on whether that is merged I'd be happy to switch this to an type alias MonthArray like you suggest

@gpbl
Copy link
Owner

gpbl commented Feb 21, 2019

Yeah it’s not that the way we are doing with those *Element props are right. Hope to fix that in the next mayor release. Thanks folks for the patience and the review!

@gpbl gpbl merged commit b0cb505 into gpbl:master Feb 21, 2019
kimamula pushed a commit to kimamula/react-day-picker that referenced this pull request Aug 17, 2022
* Add missing props from weekdayElement and caption Element

* Revert changes to docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants