adds missing tests to Page and its sub-components #526
Conversation
kvendrik
added
Testing
🤖Skip Changelog
Removed the indicator logic as its not being used right now, there is no scenario in which it was being rendered. More details on why here: Shopify/polaris-react-deprecated#1550.
kvendrik
changed the title
|
@kvendrik is this still a |
|
@danrosenthal this is ready to be reviewed, tagged @solonaarmstrong last week but looks like she hasn't had time yet. 😄 Not sure why the WIP test is still in progress it looks like its not triggering when I change the PR title. |
kvendrik
changed the title
kvendrik
changed the title
That he does! |
| @@ -0,0 +1,99 @@ | |||
| import * as React from 'react'; | |||
| import {Icon, UnstyledLink} from 'components'; | |||
There was a problem hiding this comment.
I'm not sure we can import this way? I think they need to be imported from their respective folders.
| }); | ||
|
|
||
| describe('children', () => { | ||
| it('get rendered when an icon is present', () => { |
| expect(onActionSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Should this have a test for disclosure?
There was a problem hiding this comment.
There is a test that makes sure the children get rendered when disclosure is defined (because of the if statement that controls how they get rendered based on if disclosure is defined). Apart from that the only related behaviour is the addition of an icon which is purely presentational and we therefor probably want to leave to the VRT.
| @@ -20,11 +17,9 @@ class ActionGroup extends React.Component<Props, never> { | |||
| render() { | |||
| const {actions, details, title, icon, active} = this.props; | |||
|
|
|||
| const detailsMarkup = details ? ( | |||
| const detailsMarkup = details && ( | |||
There was a problem hiding this comment.
Wasn't necessary just came past it and decided to clean it up. 😄
There was a problem hiding this comment.
I'm not clear on when we use &&. Wouldn't this need to be null if it doesn't pass the condition, since it's markup?
There was a problem hiding this comment.
Its explained more in-depth in React's docs but the short answer is that undefined is part of type ReactNode 😄 . So whenever you need to render a component conditionally instead of someCondition ? <Component /> : null you can simply do someCondition && <Component />.
|
🎉 Thanks for your contribution to Polaris React! |
WHY are these changes introduced?
fixes https://github.com/Shopify/polaris-react-deprecated/issues/2438
WHAT is this pull request doing?