Move Related Table to iot-app-kit #3
Conversation
packages/related-table/package.json
Outdated
| "singleQuote": true, | ||
| "trailingComma": "es5" | ||
| }, | ||
| "eslintConfig": { |
There was a problem hiding this comment.
We should emit all the eslint-config and just utilize what we have in the mono-repo. Goal is to not have every package contain repetive build related stuff
packages/related-table/package.json
Outdated
| "react-dom": "^16.14.0", | ||
| "styled-components": "^5.3.0" | ||
| }, | ||
| "prettier": { |
There was a problem hiding this comment.
can omit prettier config, already have a repo wide prettier config
packages/related-table/package.json
Outdated
| { | ||
| "name": "@iot-app-kit/related-table", | ||
| "version": "1.0.0", | ||
| "description": "IoT Application Kit core", |
packages/related-table/package.json
Outdated
| "@awsui/collection-hooks": "^1.0.0", | ||
| "@awsui/components-react": "^3.0.0", | ||
| "@awsui/design-tokens": "^3.0.0", | ||
| "react": "^16.14.0", |
There was a problem hiding this comment.
could we have a looser declaration of the react peer dep, similar as what's done https://github.com/react-bootstrap/react-bootstrap/blob/master/package.json#L147-L150
| }; | ||
|
|
||
| if (onReady) { | ||
| onReady({ |
There was a problem hiding this comment.
onReady is called on every render?
| ); | ||
| expect(nodesFiltered.length).toEqual(6); | ||
| }); | ||
| it('sort by name and priority', () => { |
There was a problem hiding this comment.
need some line breaks between all these test cases
| import { TableProps } from '@awsui/components-react/table'; | ||
| import { ExpandableTableNodeStatus, ITreeNode, TreeMap, TreeNode } from './TreeNode'; | ||
|
|
||
| export class TreeUtility { |
There was a problem hiding this comment.
unnecessary class wrapper
| if (value1 < value2) { | ||
| return -1; | ||
| } | ||
| // use loose comparison to handle inconsistent data types: https://issues.amazon.com/issues/AWSUI-9232 |
There was a problem hiding this comment.
reference to amzn internal documentation, need to remove internal references
| const node = new TreeNode({ name: 'Parent' }); | ||
| node.getChildren().push(childNode); | ||
|
|
||
| describe('ButtonWithTreeLines', () => { |
There was a problem hiding this comment.
unnecessary describe wrapper - if a developer runs this test suite with jest, the CLI output will already provide the context that these are the 'button with tree lines' test suite, the end effect is all test descriptions are just indented one extra level adding more noise to the developer experience
| alwaysExpanded | ||
| node={node} | ||
| content={content(node)} | ||
| onClick={() => jest.fn()} |
There was a problem hiding this comment.
why use jest.fn() here? looks this should just be () => {}
fpauer
force-pushed
the
related-table-move
branch
from
7695874 to
ad53a09
Compare
| @@ -0,0 +1,113 @@ | |||
| import { TreeLineModes } from './TreeLineModes'; | |||
There was a problem hiding this comment.
Why is TreeLineModes in its own file?
There was a problem hiding this comment.
Eventually we had some utility functions on this file but as I moved things around, it does not make sense to keep it. I will move it.
fpauer
force-pushed
the
related-table-move
branch
from
ad53a09 to
99c486d
Compare
fpauer
force-pushed
the
related-table-move
branch
from
99c486d to
325e56a
Compare
* Release 1.2.1 (#85) * fix: unsubrscribe data provider on component updates * Release 1.2.1 Co-authored-by: Norbert Nader <nnader@amazon.com> * fix: resolves #83 (#87) * fix: resolves #83 * Update index.ts Co-authored-by: Norbert Nader <nnader@amazon.com> Co-authored-by: Norbert Nader <Norbert.Nader@gmail.com> Co-authored-by: Norbert Nader <nnader@amazon.com>
Co-authored-by: Fernando Pauer <pauerf@amazon.com>
Description of changes:
Move Related Table Component to iot-app-kit
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.