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.

Sign up for GitHub

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

Fix/date picker 24hour tab order #11863

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Fix/date picker 24hour tab order #11863

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df5830a
removed is24hour class, move markup for month and day to render meth…
Nov 13, 2018
852d991
fix linting issues
Nov 13, 2018
3574784
fix linting errors
Dec 26, 2018
3dc3f74
update snapshots to test the is12Hour boolean.
Jan 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/components/src/date-time/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,6 @@
border-radius: 0 $radius-round-rectangle $radius-round-rectangle 0 !important;
}
}

&.is-24-hour {
.components-datetime__time-field-day {
order: 0 !important;
}
}
}

.components-datetime__time-legend {
Expand Down
88 changes: 50 additions & 38 deletions packages/components/src/date-time/time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class TimePicker extends Component {
this.updateMinutes = this.updateMinutes.bind( this );
this.onChangeHours = this.onChangeHours.bind( this );
this.onChangeMinutes = this.onChangeMinutes.bind( this );
this.renderMonth = this.renderMonth.bind( this );
this.renderDay = this.renderDay.bind( this );
}

componentDidMount() {
Expand Down Expand Up @@ -189,52 +191,62 @@ class TimePicker extends Component {
this.setState( { minutes: event.target.value } );
}

renderMonth( month ) {
return (
<div className="components-datetime__time-field components-datetime__time-field-month">
<select
aria-label={ __( 'Month' ) }
className="components-datetime__time-field-month-select"
value={ month }
onChange={ this.onChangeMonth }
onBlur={ this.updateMonth }
>
<option value="01">{ __( 'January' ) }</option>
<option value="02">{ __( 'February' ) }</option>
<option value="03">{ __( 'March' ) }</option>
<option value="04">{ __( 'April' ) }</option>
<option value="05">{ __( 'May' ) }</option>
<option value="06">{ __( 'June' ) }</option>
<option value="07">{ __( 'July' ) }</option>
<option value="08">{ __( 'August' ) }</option>
<option value="09">{ __( 'September' ) }</option>
<option value="10">{ __( 'October' ) }</option>
<option value="11">{ __( 'November' ) }</option>
<option value="12">{ __( 'December' ) }</option>
</select>
</div>
);
}

renderDay( day ) {
return (
<div className="components-datetime__time-field components-datetime__time-field-day">
<input
aria-label={ __( 'Day' ) }
className="components-datetime__time-field-day-input"
type="number"
value={ day }
step={ 1 }
min={ 1 }
onChange={ this.onChangeDay }
onBlur={ this.updateDay }
/>
</div>
);
}

render() {
const { is12Hour } = this.props;
const { day, month, year, minutes, hours, am } = this.state;
const renderMonthDay = [ this.renderMonth( month ), this.renderDay( day ) ];
const renderDayMonth = [ this.renderDay( day ), this.renderMonth( month ) ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is creates 4 elements by calling renderDay twice and renderMonth twice, but then only two of those elements are used.

React also warns that the elements in the array don't have keys.

Would be great to solve both things. I wouldn't be shy about extracting the day/month code out into separate component(s) if it aids clarity. It's already halfway there with the renderDay/renderMonth methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't considered it initializing those elements when the variable is assigned. I'll have to find a way to make it so that only one is instantiated when necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if doing something like:

	renderDayMonthFormat(is12Hour) {
		const { day, month } = this.state;
		const layout = [ this.renderDay( day ), this.renderMonth( month ) ];
		return is12Hour ? layout : layout.reverse();
	}

And thus:

{ this.renderDayMonthFormat(is12Hour) }

Would work better. This would cut down rendering the elements to only once. Where were you seeing React warning about the elements in the array?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like the idea of using reverse.

The warning is In the browser console—you'll need SCRIPT_DEBUG enabled in your dev environment to see it:
https://codex.wordpress.org/Debugging_in_WordPress

Some details about the particular warning from React's docs:
https://reactjs.org/docs/lists-and-keys.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @talldan I'll look into this and see what is going on! I appreciate all the help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few changes and I believe I also fixed the issue with the array keys. I looked at a few other components and created a simple key for each root element that is returned for both the day and month render methods.


return (
<div className={ classnames( 'components-datetime__time', {
'is-12-hour': is12Hour,
'is-24-hour': ! is12Hour,
} ) }>
<div className={ classnames( 'components-datetime__time' ) }>
<fieldset>
<legend className="components-datetime__time-legend invisible">{ __( 'Date' ) }</legend>
<div className="components-datetime__time-wrapper">
<div className="components-datetime__time-field components-datetime__time-field-month">
<select
aria-label={ __( 'Month' ) }
className="components-datetime__time-field-month-select"
value={ month }
onChange={ this.onChangeMonth }
onBlur={ this.updateMonth }
>
<option value="01">{ __( 'January' ) }</option>
<option value="02">{ __( 'February' ) }</option>
<option value="03">{ __( 'March' ) }</option>
<option value="04">{ __( 'April' ) }</option>
<option value="05">{ __( 'May' ) }</option>
<option value="06">{ __( 'June' ) }</option>
<option value="07">{ __( 'July' ) }</option>
<option value="08">{ __( 'August' ) }</option>
<option value="09">{ __( 'September' ) }</option>
<option value="10">{ __( 'October' ) }</option>
<option value="11">{ __( 'November' ) }</option>
<option value="12">{ __( 'December' ) }</option>
</select>
</div>
<div className="components-datetime__time-field components-datetime__time-field-day">
<input
aria-label={ __( 'Day' ) }
className="components-datetime__time-field-day-input"
type="number"
value={ day }
step={ 1 }
min={ 1 }
onChange={ this.onChangeDay }
onBlur={ this.updateDay }
/>
</div>
{ is12Hour ? renderMonthDay : renderDayMonth }
<div className="components-datetime__time-field components-datetime__time-field-year">
<input
aria-label={ __( 'Year' ) }
Expand Down