test(checkbox): Change tests to react-testing-library and improve coverage #372
Conversation
|
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 |
anicholls
force-pushed
the
testing-checkbox
branch
4 times, most recently
from
27adf62
to
75035b8
Compare
| describe('with checked=true', () => { | ||
| test('should render a checked checkbox input', () => { | ||
| const {getByLabelText} = render(<Checkbox checked={true} onChange={cb} label={label} />); | ||
| expect(getByLabelText(label)).toHaveProperty('checked', true); |
There was a problem hiding this comment.
Would love to use the new toBeChecked matcher from @testing-library/jest-dom, but there seem to be type issues within Emotion. We can update these when we update to Emotion 11 (see emotion-js/emotion#1689 for more info)
anicholls
changed the title
* feat(labs): Add bidirectionality support feat(core): Experiment bidirectionality feat(core): Rename DirectionProvider to useDirection feat(core): Use Theming for Direction feat(core): Add temporary wrapper in CanvasProvider feat(core): Add more RTL defaults to try feat(core): Use theming for Direction in more components feat(core): Set default direction to LTR feat(core): Knobs works for Direction feat(labs): Use body dir attribute instead of div wrapper feat(labs): Create overridden styled function to handle contexts feat(labs): Use new styled function to replace useDirection fix(labs): Handle undefined interpolations in styled function fix(labs): Fix types for custom styled function feat(labs): Direction from a prop in styled function fix(labs): Fix operator for direction in styled function fix(labs): Show how to use a CanvasProvider to use a different direction at a component level fix(labs): Better example fix(labs): More type fixes for custom styled function feat(labs): Add a way to pass direction prop to all styled components refactor(labs): Change extension of custom styled file to correct .ts fix(labs): Don't clone all children if direction prop isn't defined fix(labs): Follow documentation on how to create a theme fix(labs): Remove testing on components fix(labs): Code cleanup fix(labs): Code cleanup fix(labs): Update Theming Readme to include bidirectionality docs(labs): Add a story for direction feat(core): Experiment bidirectionality feat(core): Experiment bidirectionality feat(core): Rename DirectionProvider to useDirection feat(core): Use Theming for Direction feat(core): Add more RTL defaults to try feat(core): Use theming for Direction in more components feat(core): Set default direction to LTR feat(core): Knobs works for Direction feat(labs): Use body dir attribute instead of div wrapper feat(labs): Create overridden styled function to handle contexts feat(labs): Use new styled function to replace useDirection fix(labs): Handle undefined interpolations in styled function fix(labs): Fix types for custom styled function feat(labs): Direction from a prop in styled function fix(labs): Fix operator for direction in styled function fix(labs): Show how to use a CanvasProvider to use a different direction at a component level fix(labs): Better example fix(labs): More type fixes for custom styled function feat(labs): Add a way to pass direction prop to all styled components refactor(labs): Change extension of custom styled file to correct .ts fix(labs): Follow documentation on how to create a theme fix(labs): Remove testing on components fix(labs): Code cleanup fix(labs): Code cleanup fix(labs): Remove ThemeContext from popup feat(labs): Merge from theming fix(labs): Merge from theming * refactor(labs): Remove global setting functionality (deferred) * refactor(labs): Make sure props.theme is defined * feat(banner): Add theming support to try bidirectionnality * refactor(labs): Remove direction from Themeable interface * refactor(labs): Remove dir from body * fix(Labs): Prevent createCanvastheme from overriding defaultTheme * docs(labs): fix typo * docs(labs): Update README for bidirectionality * refactor(labs): Remove applyDirection * docs(Labs): Update direction story to show both directions * refactor(labs): Use bdo element instead of div for dir container * refactor(labs): Update readme and styled.ts based on PR review * refactor(labs): Code clean up * docs(labs): Fix typo * refactor(labs): Update based on PR review * chore(labs): Remove unused variable * chore(labs): Formatting updates
anicholls
force-pushed
the
testing-checkbox
branch
from
8feb830
to
b304116
Compare
Using Workday#372 as a canonical example
|
To make the visual work with the className trick would require changing the Checkbox.tsx file a bit to apply the className to both the wrapper and the container. My main question there is “should we really prevent the hover circle from showing if we are actively fighting a browser standard to do so?” If yes, we could allow the hover circle when the label is hovered. If no, maybe we should adjust the implementation of the Checkbox.tsx file slightly to allow the "hover" state to show correctly on our visual testing |
|
@NicholasBoll I've addressed both of your comments and am waiting to hear back from the design team on whether tying the ripple to the label is okay 👍 |
| * This uses our `StaticStates` system. Note: This system requires the use of Canvas' | ||
| * `styled` wrapper within the component, instead of the default emotion one. | ||
| */ | ||
| export interface StaticStatesTableProps { |
There was a problem hiding this comment.
I love the idea of this table and I think the rest of the comments in this file depend on what StaticStatesTable is intended to be, you can take these suggestions/comments with a grain of salt. Nothing blocks merging since this is only a test utility. It would be silly to be so picky for something we don't ship.
I'm approaching it as if this utility is supposed to be highly re-usable across all components in our library, not just form inputs and not just to render static states. If this wasn't the idea at all then you can probably ignore the rest of the comments or we can just simply think about this later.
To make it more generic, I would suggest that we decouple StaticStates and the table. Then you would simply compose the two and use the render prop accordingly.
// Instead of having the magic built within...
<StaticStatesTable ... />
// Compose them together instead
<StaticStates> {/* Sole job is to transform psuedo selectors to classes */}
<ComponentTable {/* renders the component with a combination of props and states */}
...
renderComponent={(...) => { return <SomeComponent ... />}}
>
</ComponentTable>
</StaticStates>
There was a problem hiding this comment.
I think more generically it would be a matrix of x props and y props. As it is now, "Props" represent a row and "state" (the className for the psuedo-selector state) are the columns.
More generically it is a matrix of prop combinations since className is really a prop.
I'm not sure I like the implementation as a permutation of props that has to be filtered. What about this instead?
<ComponentPropsTable
rowProps={[
{ label: 'Checked', checked: true, indeterminate: false },
{ label: 'Indeterminate', checked: false, indeterminate: true },
// etc
]}
columnProps={[
{ label: 'Focus', className: 'focus' },
{ label: 'Hover', className: 'hover' },
// etc
]}
>
(props) => <Checkbox {...props} />
</ComponentPropsTable>You can always make some utility functions that will give you the rowProps array from permutations
| * The render function called to render the component in each cell of the table. This gives you | ||
| * the ability to add extra styling or markup (a blue background for an inverse variant, for example). | ||
| */ | ||
| renderComponent(props: ComponentProps, disabled: boolean, className: string): React.ReactNode; |
There was a problem hiding this comment.
I would change the signature to (props: ComponentProps, state: string): React.ReactNode, getting rid of the disabled param and making the user take care of it in the render function itself. I would also rename className to state and handle that in the render function too.
This makes it a bit more generic and highly reusable for other components without a disabled state and ones that may not use StaticStates as a wrapper (we may also have other wrappers in the future) or choose to use the state for something other than a className.
Usage in https://github.com/Workday/canvas-kit/pull/372/files#diff-abfef4dc218304fbe5a1c9bb0fccb3d8R136 would look something like this:
renderComponent={(props, state) => (
<Checkbox
checked={props.checked}
disabled={state.includes('disabled')}
indeterminate={props.indeterminate}
error={props.error}
className={state}
onChange={() => {}} // eslint-disable-line no-empty-function
label="Checkbox"
/>
)}
| export default class StaticStatesTable extends React.Component<StaticStatesTableProps> { | ||
| public static defaultProps = { | ||
| componentProps: {}, | ||
| states: [ |
There was a problem hiding this comment.
If this is supposed to be more generic, I would probably just make this a required prop and maybe provide an exported collection of states for form inputs (or even make it a static member that someone can just do states={[...StaticStatesTable.States.InputStates, 'custom state']}
| })} | ||
| </tr> | ||
| </thead> | ||
| <tbody>{this.mapProps()}</tbody> |
There was a problem hiding this comment.
We should think about testing mapProps() as we use this table more. It would suck to have a bug set a false visual baseline
|
Based on conversation, we're going to address the follow up changes to |
Summary
Closes #363
Related to #286
Converts test to RTL, improves coverage, adds a static states story for visual regression testing, and uses a more BDD (behavioral-driven) approach for unit tests.
Checklist
yarn testpassespackage.json