Fixes #1947 | Create ClayTable component #2019
Conversation
diegonvs
changed the title
Pull Request Test Coverage Report for Build 2567
💛 - Coveralls |
| */ | ||
|
|
||
| import * as React from 'react'; | ||
| import * as TestRenderer from 'react-test-renderer'; |
There was a problem hiding this comment.
I think we can only use react-testing-library from now on. What do you think @bryceosterhaus?
There was a problem hiding this comment.
Yeah I think it'd be a good rule of thumb to opt for always using react-testing-library and especially testing any functional parts of the component. I'm growing less and less confident in snapshots since its so easy to just "update all"
There was a problem hiding this comment.
Yep. I think a snapshot is mostly only useful as a smoke test (ie. did it render at all?) and as an informational document of (not a meaningful assertion about) what the component currently produces.
| Head: TableHeadType; | ||
| Row: TableRowType; | ||
| } = ({ | ||
| bodyVerticalAlignment, |
There was a problem hiding this comment.
I'm just a bit reluctant with this bunch of APIs that we have to set up the classes, this is pretty cool but I'm worried because it raises our maintenance layer above the API's, that's a bit worrisome, I'd try to put it on the balance and not need to add everything we have in classes. One thing that can help us with that and add only those that the Lexicon provides and className will do the rest.
There was a problem hiding this comment.
What do you think about it @bryceosterhaus @pat270? Would be a good idea leave some APIs using css utilities on the component?
|
hey @diegonvs I would also try to narrow down the stories with the full use cases by adding many rows and columns, I think it would be enough to just show the low-level components and maybe a complete storie composing them all. Since we are discussing the high-level in #2017 we will probably have to reorganize our stories to list examples with the low-level and high-level ones. |
packages/clay-table/src/Body.tsx
Outdated
| ...otherProps | ||
| }) => { | ||
| return ( | ||
| <tbody {...otherProps} className={className}> |
There was a problem hiding this comment.
No need to declare className here. It'll already be there with ...otherProps
packages/clay-table/src/Head.tsx
Outdated
| ...otherProps | ||
| }) => { | ||
| return ( | ||
| <thead {...otherProps} className={className}> |
There was a problem hiding this comment.
no need to declare className here
packages/clay-table/src/index.tsx
Outdated
| Body: TableBodyType; | ||
| Cell: TableCellType; | ||
| Head: TableHeadType; | ||
| Row: TableRowType; |
There was a problem hiding this comment.
Rather than export these types from their components you can just do something like
Body: typeof TableBody;
Cell: typeof TableCell;
Head: typeof TableHead;
Row: typeof TableRow;
…implifies the use of component
|
I gonna reopen this PR to solve merge conflicts and git history. |
|
reopened at #2026 |
Component
Tests
Stories
Migration guide
What do think folks about using Structural testing with Storybook? I think that is another way to cover all our Jest Snapshots :)
Sometimes, for a better experience in stories, some components with state are needed. So I'm creating some of them inside
fixturesfolder. However we could have a global package for fixtures for using in storybook without publishing them. What do you think?Another question is: Would be a good practice cover all possibilities using helper classes on our Composable Components?