DatePicker - adding better year selection #580
Conversation
# Conflicts: # src/components/date-picker/calendar.tsx # test-kit/components/date-picker-driver.ts # test/components/date-picker.spec.tsx
Zombiefruit
requested review from
JoMarton,
alisey,
liorzisman and
danielstro
| private getCalendarView = () => { | ||
| if (this.state.showYearView) { | ||
| return ( | ||
| <div className="year-view" data-automation-id="YEAR_VIEW"> |
There was a problem hiding this comment.
There's a mix of camelCase and kebab-case class names in the stylesheet, I think our convention is to use camelCase.
| const yearArray: JSX.Element[] = []; | ||
|
|
||
| for (let year = this.state.viewDate.getFullYear() - 5; year <= this.state.viewDate.getFullYear() + 5; year ++) { | ||
| if (year === this.props.value.getFullYear()) { continue; } |
There was a problem hiding this comment.
Just a suggestion: maybe show the currently selected year and make it highlighted?
- It would solve the problem with variable number of years in the popup.
- Right now you need to perform two different actions depending on which year you want to select. You either need to click on the header or on the item in the list.
| private toggleMonthView = () => { | ||
| this.setState({showMonthView: !this.state.showMonthView}); | ||
| private toggleDateSelectView = () => { | ||
| if (this.state.showYearView && this.state.showMonthView) { |
There was a problem hiding this comment.
How about having only this.state.view that can be equal to year|month|default?
| } | ||
|
|
||
| private onSelectYear: React.EventHandler<React.SyntheticEvent<Element>> = event => { | ||
| event.preventDefault(); |
| export class Calendar extends React.Component<CalendarProps, CalendarState> { | ||
| public state: CalendarState = {showMonthView: false}; | ||
| export class Calendar extends React.Component<CalendarProps> { | ||
| @observable public state: CalendarState = { |
There was a problem hiding this comment.
If you want to use mobx instead of react state - please rename state to something less confusing
| @@ -55,7 +61,7 @@ export class Calendar extends React.Component<CalendarProps, CalendarState> { | |||
| <span | |||
| data-automation-id="CALENDAR_HEADER" | |||
| className="headerDate" | |||
| onMouseDown={this.headerClicked} | |||
| onMouseDown={this.onHeaderClick} | |||
There was a problem hiding this comment.
Is this span necessary to wrap the DOM you get back from getHeader()?
There was a problem hiding this comment.
Not really, but the alternative is duplicating those attributes on all three of the possible return values for getHeader
test/components/date-picker.spec.tsx
Outdated
| }); | ||
| }); | ||
|
|
||
| it('when in the year-view, it should not show the current year', async () => { |
There was a problem hiding this comment.
what should it show? why not test for positive outcome?
test/components/date-picker.spec.tsx
Outdated
| }); | ||
| }); | ||
|
|
||
| it('when in the month-view, clicking on the arrows should not change the actual date', async () => { |
There was a problem hiding this comment.
is the outcome of pressing the arrows in month view being tested anywhere? if not this is a good place for it, and during making sure the data value stays the same
| this.setState({showMonthView: !this.state.showMonthView}); | ||
| private toggleDateSelectView = () => { | ||
| if (this.state.showYearView && this.state.showMonthView) { | ||
| this.state.showYearView = false; |
There was a problem hiding this comment.
This boolean flag game could be a bit much. consider if using an enum like this.state.view = ClendarViews.Year would make this shorter and more readable
| }); | ||
| }); | ||
|
|
||
| it('when in the month-view, clicking on the arrows should not change the actual date', async () => { |
There was a problem hiding this comment.
is the outcome of pressing the arrows in month view being tested anywhere? if not this is a good place for it, and during making sure the data value stays the same
| export class Calendar extends React.Component<CalendarProps, CalendarState> { | ||
| public state: CalendarState = {showMonthView: false}; | ||
| export class Calendar extends React.Component<CalendarProps> { | ||
| @observable public calendarState: CalendarState = { |
|
|
||
| this.props.updateDropdownDate(nextDate); | ||
| if (this.calendarState.view !== 'default') { |
There was a problem hiding this comment.
Would be nice to flip the condition if (view === default) { do stuff } else { do other stuff }
| @@ -165,6 +165,10 @@ | |||
| -st-extends: calendar; | |||
| } | |||
|
|
|||
| .year-view { | |||
There was a problem hiding this comment.
Just one last detail, this should be in camel case.
Added the ability to click on the header twice, in order to see a selection of years to pick from. The user is able to then change the decade using the arrow keys.