feat: #218 v5-table-component table-part-[01] by rpt-rfoxy · Pull Request #307 · reapit/elements

rpt-rfoxy · 2025-02-10T06:32:14Z

Pull request checklist

Create table Component (table) part

Detail as per issue below (required):
as perdiscusstion here Discussion
schreenshoot

codacy-production · 2025-02-10T06:34:35Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for `d9a3443`¹	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`d9a3443`)	Report Missing	Report Missing	Report Missing
Head commit (`f5fbc46`)	4249	3854	90.70%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#307)	8	8	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

kurtdoherty

Thanks @rpt-rfoxy. I've left a few comments; I think there'll need to be a bit of conversation on some, particularly the file structure and display mode.

kurtdoherty · 2025-02-11T06:37:13Z

src/components/table/table/__tests__/table.test.tsx

+import { TableContainer } from '../../table-container'
+describe('Table', () => {
+  test('should match snapshot', () => {
+    const { asFragment } = render(


issue: It's important to design our tests to fail well. You can read more about this here. Right now, this test is will fail:

✅ when our subject changes (TableContainer); and,

❌ when one of the subject's collaborators change (e.g. Table)

Failure (2) is not what we want.

suggestion: To avoid this happening, we can simply change our test as follows:

render(<TableContainer>Fake child</TableContainer>)

This way, we're only snapshotting the subject (and it's fake child), not the subject AND a bunch of collaborators. Each collaborator will have it's own tests to validate they function correctly.

kurtdoherty · 2025-02-11T06:39:39Z

src/components/table/table/table.mdx

+
+
+The Table component can be used from the Table molecule components, e.g. `TableRow`, `TableCell`, etc
+There are two visual styles for the table, dependant on screen width.


issue: Again, I don't see anything about this kind of responsive behaviour on the Figma design spec.

kurtdoherty · 2025-02-11T06:41:38Z

src/components/table/table/styles.tsx

+  padding: 0.75rem 0.5rem;
+  ${isBelowDesktop} {
+    display: flex;
+  }


issue: I don't see anything about this kind of responsive behaviour on the Figma design spec. It's really important that we do not add complexity to our components if we're not required to by the design spec.

Unless I'm mistaken, "responsiveness" will be done by hiding/showing columns; it will not be done by adjusting the way a table's cells are laid out.

kurtdoherty · 2025-02-11T06:45:29Z

src/components/table/table/styles.tsx

+import { isBelowDesktop } from '../../../styles/media'
+import { styled } from '@linaria/react'
+
+export const ElTable = styled.table`


issue: I do not think we should be using the default table display mode. Instead, I think we should be using a flex or grid display mode. That said, I think this concern was missed on the solution design as I can't see any details around how we plan to size columns in the table correctly.

Might need further discussion on this one 🤔

kurtdoherty · 2025-02-11T06:50:18Z

src/components/table/table/table.tsx

+import React, { FC, TableHTMLAttributes } from 'react'
+import { ElTable } from './styles'
+
+export interface ITableProps extends TableHTMLAttributes<HTMLTableElement> {}


todo: Can you please keep the names for prop types and interfaces consistent with the rest of the repo? Specifically, we don't use prefixes like I to denote an interface.

kurtdoherty · 2025-02-11T06:54:01Z

src/components/table/index.ts

issue: I don't think it was discussed in the solution design, but do we really need the nested folder structure here? Feels like we could just have something like:

table/ __tests__/ table.tests.tsx table-container.tests.tsx styles.tsx table.tsx table.stories.tsx table-container.tsx table-container.stories.tsx ...

src/components/table/index.ts

rpt-rfoxy · 2025-02-11T07:26:14Z

thanks @kurtdoherty for the feedback, i'll change the code

…e interface name

kurtdoherty

Thanks @rpt-rfoxy, just the one question, otherwise looks good.

kurtdoherty · 2025-02-13T04:57:28Z

src/components/table/index.test.ts

question: why are we testing our barrel file?

its because when we do test --coverage, the result funcs always bellow 84%, so i inisiative to add test barrel file to keep the test function in 84%++

Haha, okay, that's just "gaming the system." We don't want to add tests for the sake of our coverage metrics; that approach will just lead to low-value, high-maintenance tests, and undermine the value coverage metrics are meant to provide: namely, the identification of possible risk in our code.

Instead of simply adding test coverage against a module that does not need tests, we need to assess whether there's any meaningful coverage gaps being reported. Barrel files are not a meaningful gap.

To help with this, I'll make a change to our coverage settings to exclude barrel files. Once that's merged in, rebase you changes and see what your coverage comes back as for the components you're adding in this PR.

I've raised #315 👍

#315 has now been merged.

ksolanki7 · 2025-02-13T22:55:45Z

src/index.ts

 export * from './components/skeleton'
 export * from './components/tooltip'
-export * from './components/table/table-container'
-export * from './components/table/table-toolbar'


@rpt-rfoxy question: Is there a reason why this is being removed now? We agreed upon having separate folders for atoms. This will allow you to avoid having a barrel file.
@kurtdoherty Please do correct me if I am wrong here.

I didn't notice this change sorry 🙈 I agree with you, removing these doesn't seem to be necessary.

thanks for the feedback, i already remove the barrel

feat: #218 v5-table-component table-part

a813006

rpt-rfoxy requested review from ksolanki7 and kurtdoherty February 10, 2025 06:32

rpt-rfoxy changed the title ~~feat: #218 v5-table-component table-part~~ feat: #218 v5-table-component table-part-[01] Feb 10, 2025

feat: #218 v5-table-component update export file

6b11aea

rpt-rfoxy mentioned this pull request Feb 10, 2025

feat: #218 v5-table-component refactoring code #301

Closed

6 tasks

kurtdoherty suggested changes Feb 11, 2025

View reviewed changes

foxy added 2 commits February 12, 2025 13:51

feat: #272 v5-label-text component update the test file and rename th…

f482dd5

…e interface name

feat: #272 v5-label-text component fix linter

7afa360

rpt-rfoxy requested a review from kurtdoherty February 12, 2025 07:34

rpt-rfoxy and others added 2 commits February 12, 2025 14:34

Merge branch 'main' into feat-218-v5-table-component-part-table

8613e65

eat: #272 v5-label-text component fix test coverage in table

dab7042

kurtdoherty approved these changes Feb 13, 2025

View reviewed changes

kurtdoherty mentioned this pull request Feb 13, 2025

chore: exclude barrel files from coverage reporting #315

Merged

ksolanki7 reviewed Feb 13, 2025

View reviewed changes

foxy and others added 3 commits February 18, 2025 06:53

feat: 218-v5-table-component remove barrel

6484e7b

Merge branch 'main' into feat-218-v5-table-component-part-table

692b7b8

feat: 218-v5-table-component update snapshot

36302eb

kurtdoherty approved these changes Feb 19, 2025

View reviewed changes

foxy and others added 2 commits February 20, 2025 06:15

feat: 218-v5-table-component update documentation description

41c3bf5

Merge branch 'main' into feat-218-v5-table-component-part-table

0eb1dfd

kurtdoherty approved these changes Feb 26, 2025

View reviewed changes

Merge branch 'main' into feat-218-v5-table-component-part-table

f5fbc46

rpt-rfoxy merged commit 9f976ee into main Mar 3, 2025
6 checks passed

rpt-rfoxy deleted the feat-218-v5-table-component-part-table branch March 3, 2025 00:32



		The Table component can be used from the Table molecule components, e.g. `TableRow`, `TableCell`, etc
		There are two visual styles for the table, dependant on screen width.

Conversation

rpt-rfoxy commented Feb 10, 2025

Pull request checklist

Uh oh!

codacy-production bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage summary from Codacy

Footnotes

Uh oh!

kurtdoherty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rpt-rfoxy commented Feb 11, 2025

Uh oh!

kurtdoherty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kurtdoherty Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codacy-production bot commented Feb 10, 2025 •

edited

Loading

kurtdoherty Feb 13, 2025 •

edited

Loading