Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 20, 2021

WHY are these changes introduced?

I am updating existing tests for the following components in Card component to use the new modern framework:

  • Header
  • Section
  • Subsection
  • Card

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 force-pushed the polaris_card branch from 0d1044a to 834757a Compare July 20, 2021 03:30
@github-actions
Copy link
Contributor

size-limit report

Path Size
cjs 142.12 KB (0%)
esm 95.85 KB (0%)
esnext 138.85 KB (0%)
css 33.56 KB (0%)

@ghost ghost requested review from a team and AndrewMusgrave July 20, 2021 12:11
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Left a couple comments about findAll but other than that, LGTM to me 💯

Comment on lines 155 to 156
const buttons = card.findAll(Button);
expect(buttons[0].prop('children')).toBe('test action');
Copy link
Member

Choose a reason for hiding this comment

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

Curious why we're finding all but only using the first? It could be written as

const button = card.find(Button);
expect(button.prop('children')).toBe('test action');

or

Suggested change
const buttons = card.findAll(Button);
expect(buttons[0].prop('children')).toBe('test action');
expect(card).toContainReactComponent(Button, {children: 'test action'});

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!

expect(card.find(Popover).first()).toHaveLength(0);
const buttons = card.findAll(Button);
expect(buttons[0].prop('children')).toBe('test action');
expect(card).not.toContainReactComponent(Popover);
Copy link
Member

Choose a reason for hiding this comment

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

^

@ghost ghost force-pushed the polaris_card branch from 834757a to 60eeece Compare July 27, 2021 21:45
@ghost ghost force-pushed the polaris_card branch from 60eeece to af37a5e Compare July 27, 2021 21:55
@ghost ghost merged commit 4db0181 into main Jul 27, 2021
@ghost ghost deleted the polaris_card branch July 27, 2021 22:03
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