Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 18, 2021

WHY are these changes introduced?

I am updating existing tests for UnstyledLink, Tag, DisplayText, FileUpload, MessageIndicator, Choice and Dialog components to use the new modern framework.

Thanks @rdott for the suggestion on the tag tests!

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'.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2021

size-limit report

Path Size
cjs 142.64 KB (0%)
esm 96.38 KB (0%)
esnext 139.58 KB (0%)
css 33.76 KB (0%)

@ghost ghost force-pushed the polaris_modals branch from 6854ce1 to 98deb99 Compare August 18, 2021 16:42
@ghost ghost changed the title [TestModernization]: Add test modernization to DisplayText, FileUpload, MessageIndicator and Dialog [TestModernization]: Add test modernization to UnstyledLink, Tag, DisplayText, FileUpload, MessageIndicator, Choice and Dialog Aug 18, 2021
@ghost ghost force-pushed the polaris_modals branch from 98deb99 to ccd6d07 Compare August 18, 2021 19:23
@ghost ghost marked this pull request as ready for review August 18, 2021 19:23
@ghost ghost requested review from a team and rdott August 19, 2021 15:54
const spy = jest.fn();

const dialog = mountWithApp(
<Dialog labelledBy="test" onClose={jest.fn()} onEntered={spy}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does onClose need to be mocked here? Could this be the following (just to match with the patterns in other tests)?

onClose={noop}

Adding a

function noop() {}

To the end of the test file?

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking that, but because it is not needed on the test, I thought to leave it like it. But I can add the noop one!

Copy link
Contributor

@rdott rdott 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.

Couple of small (non-blocking) suggestions.

🎉

@ghost ghost force-pushed the polaris_modals branch from 7e039e5 to 3a7bf44 Compare August 23, 2021 12:00
@ghost ghost merged commit eb38ad0 into main Aug 23, 2021
@ghost ghost deleted the polaris_modals branch August 23, 2021 18:07
This pull request was closed.
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.

2 participants