feat: Table cell align for all cells #3318
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3318 +/- ##
==========================================
+ Coverage 96.42% 96.44% +0.01%
==========================================
Files 791 795 +4
Lines 22616 22711 +95
Branches 7820 7848 +28
==========================================
+ Hits 21808 21903 +95
Misses 754 754
Partials 54 54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pan-kot
force-pushed
the
table-all-cell-align
branch
2 times, most recently
from
0e27353 to
00391bc
Compare
pan-kot
force-pushed
the
table-all-cell-align
branch
from
00391bc to
dce402f
Compare
| * Determines the alignment of the content inside table cells. | ||
| * This property affects all cells, including the ones in the selection column. | ||
| * To target individual cells use `columnDefinitions.verticalAlign`. | ||
| */ |
There was a problem hiding this comment.
I think it would be a good idea to mention which one takes precedence here.
There was a problem hiding this comment.
I think the columnDefinitions.verticalAlign taking precedence is the only reasonable assumption because otherwise it won't work at all if cellVerticalAlign is set.
There was a problem hiding this comment.
I think that's reasonable to assume but making it explicit and clear would be even better in my opinion.
src/table/__tests__/table.test.tsx
Outdated
| import headerCellStyles from '../../../lib/components/table/header-cell/styles.css.js'; | ||
| import styles from '../../../lib/components/table/styles.css.js'; | ||
|
|
||
| const useBaseComponentSpy = jest.spyOn(baseComponentHooks, 'default'); |
There was a problem hiding this comment.
Would it make sense to mock the dependencies (e.g. useComponentMetrics and useComponentMetadata) rather than an internal hook? The reason for this is that if we break something inside the internal hook this test will not be catching the regression.
There was a problem hiding this comment.
This test is not intended to capture possible issues inside the useBaseComponent, but only the props that we pass to it from the table. If we mock the inner hooks instead, that would be a violation of the hook's encapsulation. Instead, we treat it as a black box.
There was a problem hiding this comment.
This mock does not only apply to your new tests, but for all tests in this file, which may lead to coverage issues.
If you want to test metrics reporting specifically, extract this into a separate table-metrics.test.tsx, do not do this into the main test file
| jest.mock('../../../lib/components/internal/hooks/use-mobile', () => ({ | ||
| useMobile: jest.fn(), | ||
| })); |
There was a problem hiding this comment.
Why is this mock needed?
Evergreen rule – less mocks, better tests
See this PR for an example why mocks are bad: #3277
There was a problem hiding this comment.
That was a copy-paste artifact from the table tests file, it is not needed for the metrics tests - removed.
3 participants
Description
This PR introduces a new API property for the table component, the:
cellVerticalAlign?: "middle" | "top". This property will complement the existingcolumnDefinitions.verticalAlign?: "middle" | "top"property, but is applicable for the entire table, including the selection column. When both properties are set, thecolumnDefinitions.verticalAligntakes precedence.On the screenshot you can see how selection checkboxes are aligned to the cell's top when
cellVerticalAlign = "top". The last column alignment is overridden by setting thecolumnDefinitions.verticalAlign = "middle".Rel: AWSUI-59979
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.