Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Sep 1, 2021

WHY are these changes introduced?

I am updating existing tests for TextField components to use the new modern framework.

WHAT is this pull request doing?

Part of test modernization (https://docs.google.com/spreadsheets/d/1GBuEZbOpVYJLNISK7gL69DU8rCxocn5L5GYaKtPXPbU/edit#gid=1498187033), updating tests using {mountWithAppProvider} from 'test-utilities/legacy' to {mountWithApp} from 'test-utilities'.

@ghost ghost changed the title Add textfield tests [TestModernization]: Add test modernization to TextField Sep 1, 2021
@ghost ghost marked this pull request as ready for review September 3, 2021 02:05
@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2021

size-limit report

Path Size
cjs 163.46 KB (0%)
esm 96.19 KB (0%)
esnext 143.09 KB (0%)
css 34.52 KB (0%)

@rdott rdott requested review from a team and voiid September 3, 2021 15:11
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Looks good but I'm suspicious about the find('div', {role: 'button'{) bit. If you have context on how that role is getting passed to a div please let me know

Comment on lines +647 to +648
.find('div', {
role: 'button',
Copy link
Member

@kyledurand kyledurand Sep 3, 2021

Choose a reason for hiding this comment

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

I find this a bit jarring. Why would a div have a role of button on it? Looking at the TextField component it looks like the role doesn't get passed to the div with the onClick handler?

Copy link
Contributor

@rdott rdott Sep 3, 2021

Choose a reason for hiding this comment

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

I was not 100% of the mechanics of it getting passed down. This solution was from converting

element.find('[role="button"]').first().simulate('click');

from the legacy suite and then debugging to find the specific element that the above (and similar) are targeting.

The debug output for these (console.log(element.find('[role="button"]').first().debug());) returns the div element with this role:

<div role="button" ...

Looking at it closer, it looks like this is rendered by the Spinner component, which accepts a handleNumberChange callback, which itself accepts a number (1 for increment, -1 for decrement) and then calls the onChange which is being tested.

I believe the role is to indicate the div is being used a button.

One option would be to refactor these test selectors to directly call onChange on the <Spinner> rendered in the TestField, as reaching down into the sub-component feels like behaviour that is/should be tested within the Spinner itself.

e.g.

element.find(Spinner)!.trigger('onChange', 1);

This proposed change would most likely require additional changes in Spinner.test.tsx tests, which currently tests the callback props are called, but does not test if they are called with any of the hardcoded values defined on the Spinner div buttons(1 or -1).

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, thanks for looking into that @rdott ❤️ that makes sense. I just did a test with my screenreader and all is well. :shipit: !

@rdott rdott merged commit 26e9e38 into main Sep 3, 2021
@rdott rdott deleted the polaris_textfield branch September 3, 2021 19:54
@kyledurand kyledurand mentioned this pull request Sep 3, 2021
6 tasks
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.

3 participants