Add showAllWeekdayLabels option to show all weekdays. #87
Conversation
panuhorsmalahti
commented
Aug 21, 2018
panuhorsmalahti
commented
- Fix JSDom 11.12 causes SecurityError: localStorage is not available for opaque origins jestjs/jest#6766 jest error (which caused tests to fail)
- Add showAllWeekdayLabels, which defaults to false. If true, every weekday label is displayed
- Add tests for the new feature
- Fixes one issue mentioned in Start week on Monday instead of Sunday #78
There was a problem hiding this comment.
@panuhorsmalahti thanks for the PR and sorry it sat for so long. I like this implementation more than the one in #84 because it wouldn't be a breaking change. Can you look at the review comments and rebase master please and then we'll get this merged + released?
| @@ -54,5 +54,8 @@ | |||
| "rimraf": "^2.0.0", | |||
| "webpack": "^3.0.0", | |||
| "webpack-dev-server": "^2.0.0" | |||
| }, | |||
| "jest": { | |||
| "testURL": "http://localhost" | |||
There was a problem hiding this comment.
can this be in jest.config.js instead?
| const calendar = shallow(<CalendarHeatmap | ||
| startDate={dateNDaysAgo(7)} | ||
| values={[]} | ||
| showWeekdayLabels={true} |
There was a problem hiding this comment.
do you think folks should have to pass both showWeekdayLabels and showAllWeekdayLabels or should just showAllWeekdayLabels be enough? I like the idea of just passing showAllWeekdayLabels by itself rather than 2 props, I think you have to update this line https://github.com/patientslikeme/react-calendar-heatmap/pull/87/files#diff-73642d587163f2eb4d72f0fa6ebfba02L293 to make a single prop enough
| @@ -333,6 +333,7 @@ CalendarHeatmap.propTypes = { | |||
| horizontal: PropTypes.bool, // whether to orient horizontally or vertically | |||
| showMonthLabels: PropTypes.bool, // whether to show month labels | |||
| showWeekdayLabels: PropTypes.bool, // whether to show weekday labels | |||
| showAllWeekdayLabels: PropTypes.bool, // whether to show all weekday labels | |||
There was a problem hiding this comment.
can you also document this prop in the readme?
