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

Fix/date picker 24hour tab order #11863

Merged
merged 4 commits into from Jan 22, 2019

Conversation

Projects
None yet
5 participants
@LukePettway
Copy link
Contributor

LukePettway commented Nov 14, 2018

Description

In #11566 it is mentioned that when you choose a date format such as j. F Y or 13 November 2018 the post publish date and time picker displays the order correctly to user but the tab order isn't logical. This is a result of flexbox being used to change the visual order of the input controls on the page.

The easiest and most likely "best" way to fix this is to render the markup in the logical order instead of relying on flexbox styling. It does create a little more code and logic within the component but completely mitigates the issues that could be associated with doing something like juggling the tabindex value for each input.

For some reason I couldn't get unit tests to run, I kept getting an error when trying to update snapshot tests. I think I may need to reinstall my dependencies to resolve that problem.

I also find it strange that I need to set the time format to 24 hour to get the date to display in this order, does that sound right?

I'm open to suggestions about how to better handle swapping the order of the date and month within the React component render cycle. I toyed with the idea of just reversing the array of render methods to change the order but opted to be a little more verbose.

How has this been tested?

I tested this in multiple browsers by changing the date format back and forth between the US and rest of the worlds standard formats. Then I used my keyboard to tab through the inputs to make sure the order is correct. Finally I changed the date multiple times to make sure none of the functionality was lost.

Screenshots

Here is the tab order of the non-US date format displayed in the correct order and tabbing correctly
gif of date month format tab order in date time element

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Luke Pettway added some commits Nov 13, 2018

Luke Pettway
removed is24hour class, move markup for month and day to render metho…
…ds and use some logic to determine logical order
Luke Pettway
@brendanmckeown

This comment has been minimized.

Copy link

brendanmckeown commented Nov 15, 2018

I also find it strange that this logic is based upon the time format. Shouldn't it be based upon the date format instead? Besides that basis, I think the logic itself is a nice way to resolve this tabbing order issue.

@tofumatt tofumatt self-requested a review Nov 28, 2018

@noisysocks noisysocks added this to the 4.8 milestone Dec 10, 2018

@talldan
Copy link
Contributor

talldan left a comment

Hey, thanks for looking into this and coming up with a fix. The React warning is something that'll need to be solved, but it seems to be on the right track.

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 ) ];

This comment has been minimized.

@talldan

talldan Dec 14, 2018

Contributor

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.

This comment has been minimized.

@LukePettway

LukePettway Dec 14, 2018

Author Contributor

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.

This comment has been minimized.

@LukePettway

LukePettway Dec 14, 2018

Author Contributor

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?

This comment has been minimized.

@talldan

talldan Dec 17, 2018

Contributor

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

This comment has been minimized.

@LukePettway

LukePettway Dec 17, 2018

Author Contributor

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

This comment has been minimized.

@LukePettway

LukePettway Dec 26, 2018

Author Contributor

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.

@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 21, 2018

Luke Pettway
@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Dec 31, 2018

@LukePettway - thanks for addressing the feedback 😄

It looks like there's a test failing that's related to these changes:

TimePicker › should have the correct CSS class if 12-hour clock is specified
[1] 
[1]     expect(received).toBe(expected) // Object.is equality
[1] 
[1]     Expected: true
[1]     Received: false
[1] 
[1]       13 | 		const onChangeSpy = jest.fn();
[1]       14 | 		const picker = shallow( <TimePicker currentTime="1986-10-18T11:00:00" onChange={ onChangeSpy } is12Hour={ true } /> );
[1]     > 15 | 		expect( picker.hasClass( 'is-12-hour' ) ).toBe( true );
[1]          | 		                                          ^
[1]       16 | 	} );
[1]       17 | 
[1]       18 | 	it( 'should have the correct CSS class if 24-hour clock is specified', () => {
[1] 
[1]       at Object.toBe (packages/components/src/date-time/test/time.js:15:45)
[1] 
[1]   ● TimePicker › should have the correct CSS class if 24-hour clock is specified
[1] 
[1]     expect(received).toBe(expected) // Object.is equality
[1] 
[1]     Expected: true
[1]     Received: false
[1] 
[1]       21 | 			<TimePicker currentTime="1986-10-18T11:00:00" onChange={ onChangeSpy } is12Hour={ false } />
[1]       22 | 		);
[1]     > 23 | 		expect( picker.hasClass( 'is-24-hour' ) ).toBe( true );
[1]          | 		                                          ^
[1]       24 | 	} );
[1]       25 | 
[1]       26 | 	it( 'should call onChange with an updated day', () => {
[1] 
[1]       at Object.toBe (packages/components/src/date-time/test/time.js:23:45)

Are you able to take a look at that? Thanks.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Dec 31, 2018

@LukePettway - thanks for addressing the feedback 😄

It looks like there's a test failing that's related to these changes:

TimePicker › should have the correct CSS class if 12-hour clock is specified
[1] 
[1]     expect(received).toBe(expected) // Object.is equality
[1] 
[1]     Expected: true
[1]     Received: false
[1] 
[1]       13 | 		const onChangeSpy = jest.fn();
[1]       14 | 		const picker = shallow( <TimePicker currentTime="1986-10-18T11:00:00" onChange={ onChangeSpy } is12Hour={ true } /> );
[1]     > 15 | 		expect( picker.hasClass( 'is-12-hour' ) ).toBe( true );
[1]          | 		                                          ^
[1]       16 | 	} );
[1]       17 | 
[1]       18 | 	it( 'should have the correct CSS class if 24-hour clock is specified', () => {
[1] 
[1]       at Object.toBe (packages/components/src/date-time/test/time.js:15:45)
[1] 
[1]   ● TimePicker › should have the correct CSS class if 24-hour clock is specified
[1] 
[1]     expect(received).toBe(expected) // Object.is equality
[1] 
[1]     Expected: true
[1]     Received: false
[1] 
[1]       21 | 			<TimePicker currentTime="1986-10-18T11:00:00" onChange={ onChangeSpy } is12Hour={ false } />
[1]       22 | 		);
[1]     > 23 | 		expect( picker.hasClass( 'is-24-hour' ) ).toBe( true );
[1]          | 		                                          ^
[1]       24 | 	} );
[1]       25 | 
[1]       26 | 	it( 'should call onChange with an updated day', () => {
[1] 
[1]       at Object.toBe (packages/components/src/date-time/test/time.js:23:45)

Are you able to take a look at that? Thanks.

Sure thing! I was having issues getting unit tests to run so I'm currently looking into why that is and once I figure out the issue, I can update the failing tests.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 2, 2019

Cool, let me know if you need a hand with anything.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 2, 2019

@talldan I do have one question about the tests for this. It currently fails because it expects that class to be added which I removed since all it was doing was using flexbox to change the order.

Now that it can render the markup in two different ways, should I write test cases for each of those? Or is simply having it check that it renders at all more than suitable enough for now?

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 4, 2019

@LukePettway Replacing the tests with new ones to make sure the rendered order of the components is correct would be great.

Are you familiar with snapshot tests? That might be the easiest way to go:

const wrapper = shallow( <TimePicker currentTime="1986-10-18T23:00:00" is12Hour /> );
expect( wrapper ).toMatchSnapshot();

One test case where 12 hour is true, and one where it's false will catch any potential future regressions.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 4, 2019

@LukePettway Replacing the tests with new ones to make sure the rendered order of the components is correct would be great.

Are you familiar with snapshot tests? That might be the easiest way to go:

const wrapper = shallow( <TimePicker currentTime="1986-10-18T23:00:00" is12Hour /> );
expect( wrapper ).toMatchSnapshot();

One test case where 12 hour is true, and one where it's false will catch any potential future regressions.

Yep! I am very familiar with snapshots, I can make these two test cases. Thanks very much

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 6, 2019

@talldan For some reason I can't seem to get any of the tests to work. When I run npm test I get hundreds of code formatting errors and when I just run tests for just, every test case fails. Any idea what I could be doing wrong?

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 7, 2019

@LukePettway npm test is defined as npm run lint && npm run test-unit, so I think what you're seeing must be the output from linting. It's strange that you're seeing so many errors though, I only see a few warnings about skipped tests on my machine.

Maybe use npm run test-unit or npm run test-unit:watch (which won't run linting) for these tests to see if you get the same issue. That should also help you write the tests and generate the fixtures.

There are quite a few other places that linting is checked (commit hooks, CI jobs, code reviews) so I wouldn't be concerned that there are any formatting errors, it's probably something related to how eslint is configured on your system.

edit: Sorry, just re-read your message and see that you've probably tried npm run test-unit. Not sure what could be wrong. Maybe try cleaning and reinstalling node modules using git clean -xdf && npm install (make sure you've stashed or committed changes first), and then trying again.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 8, 2019

@talldan Thanks for the suggestion. I tried this along with a few other things and I still run into the same problem.

The output I get is as follows:

    The name `@wordpress/jest-console` was looked up in the Haste module map. It cannot be resolved, because there exists several different files, or packages, that provide a module for that particular name and platform. The platform is generic (no extension). You must delete or blacklist files until there remains only one of these:

      * `/Users/lukepettway/code/gutenberg/gutenberg-mobile/gutenberg/gutenberg-mobile/gutenberg/packages/jest-console/package.json` (package)
      * `/Users/lukepettway/code/gutenberg/gutenberg-mobile/gutenberg/packages/jest-console/package.json` (package)
      * `/Users/lukepettway/code/gutenberg/packages/jest-console/package.json` (package)

    > 1 | require( '@wordpress/jest-console' );
        | ^

I wonder if I just need to do a fresh install of Gutenberg at this point.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 9, 2019

@LukePettway Ah yes, I remember this error message, it was related to the removal of the mobile submodule. IIRC the solution is to remove the gutenberg-mobile folder.

Though I would've expected git clean -xdf to have done that.

Some discussion on slack happened back here if it helps:
https://wordpress.slack.com/archives/C02QB2JS7/p1541619075024100

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 9, 2019

@talldan removing that directory worked! Not sure how it was created but the tests are working and I can finally work on getting the tests updated. Thank you so much for all your time on this.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 14, 2019

@talldan I've been trying to come up with a way to test this and it's proving to be harder than I thought. Any ideas on what would be the best way to test something like this?

At first I figured I could just look at the render order, but there doesn't seem to be a simple way of testing that without doing a lot of querying through the node tree to see which control is rendered first. I also want to avoid adding class names just for the sake of testing.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 15, 2019

Hey @LukePettway - how did it go with the snapshot tests? Expanding on the example from an earlier comment (#11863 (comment)), something like this should do the job:

it( 'matches the snapshot when the is12hour prop is specified', () => {
  const wrapper = shallow( <TimePicker currentTime="1986-10-18T23:00:00" is12Hour /> );
  expect( wrapper ).toMatchSnapshot();
} );

it( 'matches the snapshot when the is12hour prop is undefined', () => {
  const wrapper = shallow( <TimePicker currentTime="1986-10-18T23:00:00" /> );
  expect( wrapper ).toMatchSnapshot();
} );

The wrapper should render deeply enough so that the day and month inputs are in the snapshot.

@LukePettway

This comment has been minimized.

Copy link
Contributor Author

LukePettway commented Jan 15, 2019

@talldan I was able to achieve what I think works for testing this. I think I was trying to make it more complicated than it needed to be. Thanks 👍 !

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 21, 2019

Thanks for the hard work on this @LukePettway!

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 22, 2019

Merging this now as there's no further comments.

@talldan talldan merged commit 705e57e into WordPress:master Jan 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment