test(popup): Add tests for Popup #600
Conversation
| @@ -152,7 +152,6 @@ export default class Popup extends React.Component<PopupProps> { | |||
| size={closeIconSize} | |||
| onClick={handleClose} | |||
| icon={xIcon} | |||
| title={closeLabel} | |||
There was a problem hiding this comment.
We already have an aria label which is why removed the title, thoughts?
There was a problem hiding this comment.
I think it was added so that a mouse user could see what the Icon would do? I agree we don't want both title and aria-label. Perhaps this should be a label Tooltip? The new Tooltip API only exists on the v4 branch, but would be what we want.
It would look something like:
<Tooltip title={closeLabel}>
<IconButton
/* ... */
/>
The label is actually required on the tooltip as well as the `IconButton` because of Typescript. The Tooltip uses `React.cloneElement` and adds an `aria-label` to the child it wraps.
</Tooltip>
| @@ -41,8 +41,8 @@ describe('Toast', () => { | |||
| {toastMessage} | |||
| </Toast> | |||
| ); | |||
| const closeIcon = container.querySelector('[data-close="close"]'); /*? */ | |||
| fireEvent.click(closeIcon); /*? */ | |||
There was a problem hiding this comment.
this was some comments that where left, seemed minor enough to remove
There was a problem hiding this comment.
I think these are also removed in v4
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
|
||
| function getCloseButton() { | ||
| return getPopup().find('[data-close]'); | ||
| } |
There was a problem hiding this comment.
this function and the one above are similar to the modal helper functions, should I use those instead?
There was a problem hiding this comment.
Hmm. If I were to share some helpers I might do the opposite since Popup is what's common between Popup and Modal. But that's an implementation detail of the Modal. Modal uses shared helpers - those helper functions are meant to be usable eventually outside of this repository. Non-sharable helper functions should remain inside the spec file IMO.
These helper functions are different than the modal ones. They don't map as closely to the Cypress command log - these are more like "macro helpers" than just call to other commands and don't have their own logging. This isn't a huge problem on simpler tests, but can really aid in debugging larger tests.
There was a problem hiding this comment.
ah gotcha, thanks for explaining that!
| @@ -35,6 +35,9 @@ | |||
| "workday", | |||
| "popup" | |||
| ], | |||
| "devDependencies": { | |||
| "@workday/canvas-kit-labs-react-core": "^3.6.0" | |||
| }, | |||
There was a problem hiding this comment.
used for stories using static states component
|
Some things to note about our Popup. Our popup is more of a styled container, our Modal component has more accessibility and functionality than our popup. After talking to Nicholas for a bit, this component seems more like a dialog container. I wrote some minimal cypress tests and wanted to get some clarification if they make sense @NicholasBoll |
| @@ -26,7 +26,7 @@ export interface PopupProps extends React.HTMLAttributes<HTMLDivElement> { | |||
| * The origin from which the Popup will animate. | |||
| * @default {horizontal: 'center', vertical: 'top'} | |||
| */ | |||
| transformOrigin: TransformOrigin; | |||
| transformOrigin: TransformOrigin | null; | |||
There was a problem hiding this comment.
this allows to pass null in the visual stories and doesn't add the animation allowing for the snap shots to be captured
Summary
Adds visual and cypress test for Popup and change enzyme test to react testing library