feat(pds-table): add head border, background, and row divider props#629
feat(pds-table): add head border, background, and row divider props#629pixelflips merged 19 commits intomainfrom
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
bot
added
the
package: core
…lean up unused code
… streamline rowDividers logic
…pdate documentation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
QuintonJason
left a comment
QuintonJason
left a comment
There was a problem hiding this comment.
Nice table update 🎉
📝 WalkthroughWalkthroughThis pull request adds three public boolean props: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx (1)
196-196: Redundant ternary expression.Both branches of the ternary return
'checkbox-cell', making the condition pointless. Simplify or complete the intended logic.Suggested fix
- <pds-table-cell part={this.tableRef.fixedColumn ? 'checkbox-cell' : 'checkbox-cell'} class={this.tableRef.selectable ? 'has-checkbox' : ''} > + <pds-table-cell part="checkbox-cell" class={this.tableRef.selectable ? 'has-checkbox' : ''} >
🤖 Fix all issues with AI agents
In
@libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx:
- Around line 80-101: The test in pds-table-row.spec.tsx verifies the
has-divider class but misses asserting that the last row gets the special
is-last-row class (and earlier rows do not); update the test for the
PdsTable/PdsTableRow spec that builds `page` and `rows` to add assertions
checking `rows?.[rows.length - 1]?.classList.contains('is-last-row')` is true
and `rows?.[0]?.classList.contains('is-last-row')` is false so the last row is
marked correctly and earlier rows are not.
🧹 Nitpick comments (5)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
62-69: Consider expanding test coverage for the rowDividers feature.The test correctly verifies that the
row-dividersattribute is present, but could be enhanced to validate the actual behavior:
- Verify the attribute value is "true"
- Check that child
pds-table-rowelements receive the appropriate classes (e.g.,has-divider,is-last-row)- Confirm that divider styling is applied correctly
💡 Example enhanced test
it('renders with rowDividers prop set', async () => { const page = await newSpecPage({ - components: [PdsTable], + components: [PdsTable, PdsTableBody, PdsTableRow, PdsTableCell], - html: `<pds-table row-dividers="true" component-id="test-table"></pds-table>`, + html: ` + <pds-table row-dividers="true" component-id="test-table"> + <pds-table-body> + <pds-table-row> + <pds-table-cell>Row 1</pds-table-cell> + </pds-table-row> + <pds-table-row> + <pds-table-cell>Row 2</pds-table-cell> + </pds-table-row> + </pds-table-body> + </pds-table> + `, }); expect(page.root).toHaveAttribute('row-dividers'); + expect(page.root.getAttribute('row-dividers')).toBe('true'); + + // Verify that rows receive the has-divider class + const rows = page.root.querySelectorAll('pds-table-row'); + rows.forEach((row, index) => { + expect(row.classList.contains('has-divider')).toBe(true); + if (index === rows.length - 1) { + expect(row.classList.contains('is-last-row')).toBe(true); + } + }); });libs/core/src/components/pds-table/pds-table-head/test/pds-table-head.spec.tsx (1)
132-155: Consider expanding test coverage for border and background props.The tests verify attribute presence but could be more thorough:
- Verify the actual attribute values (e.g.,
expect(page.root).toHaveAttribute('border', 'true')vs. just presence).- Test behavior when props are explicitly set to
false.- Consider testing the visual impact via snapshot tests or class presence on child elements.
💡 Enhanced test coverage
+ it('does not render border attribute when border prop is false', async () => { + const page = await newSpecPage({ + components: [PdsTableHead], + html: `<pds-table-head border="false"></pds-table-head>`, + }); + expect(page.root).not.toHaveAttribute('border'); + }); + + it('does not render background attribute when background prop is false', async () => { + const page = await newSpecPage({ + components: [PdsTableHead], + html: `<pds-table-head background="false"></pds-table-head>`, + }); + expect(page.root).not.toHaveAttribute('background'); + });libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.tsx (1)
80-98: Consider removing the defensive guard.The defensive disconnect at lines 84-86 appears unnecessary since
componentDidLoadonly executes once per component instance. However, this defensive coding doesn't harm functionality.♻️ Optional simplification
// Watch for changes to the parent table-head's border and background attributes const tableHead = this.hostElement.closest('pds-table-head') as HTMLElement; if (tableHead && typeof MutationObserver !== 'undefined') { - // Defensive guard: disconnect existing observer before creating a new one - if (this.headObserver) { - this.headObserver.disconnect(); - } - this.headObserver = new MutationObserver(() => { // Update state when border or background attributes change this.hasHeadBorder = tableHead.hasAttribute('border');libs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsx (1)
366-401: Test coverage is adequate but could be strengthened.The test verifies that the
MutationObserveris set up and that attributes are applied, but the comment at line 398-399 indicates uncertainty about whether state updates trigger re-renders in the test environment. This is acceptable for integration testing, though a more direct unit test of the observer callback might provide stronger verification.libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx (1)
165-177: O(n²) complexity when rows are reordered.Each row's
bodyObservertriggersupdateLastRowState()which queries all rows viaquerySelectorAll. When N rows change simultaneously (e.g., sorting), this results in N×N operations. For typical table sizes this is acceptable, but for large tables (100+ rows) it could cause jank.Consider deferring updates with a microtask or having the parent table notify rows directly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
libs/core/src/components.d.tslibs/core/src/components/pds-table/docs/pds-table.mdxlibs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.scsslibs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.tsxlibs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsxlibs/core/src/components/pds-table/pds-table-head/pds-table-head.scsslibs/core/src/components/pds-table/pds-table-head/pds-table-head.tsxlibs/core/src/components/pds-table/pds-table-head/readme.mdlibs/core/src/components/pds-table/pds-table-head/test/pds-table-head.spec.tsxlibs/core/src/components/pds-table/pds-table-row/pds-table-row.scsslibs/core/src/components/pds-table/pds-table-row/pds-table-row.tsxlibs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsxlibs/core/src/components/pds-table/pds-table.tsxlibs/core/src/components/pds-table/readme.mdlibs/core/src/components/pds-table/stories/pds-table.stories.jslibs/core/src/components/pds-table/test/pds-table.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
libs/react/src/components/proxies.ts (1)
PdsTable(91-91)
libs/core/src/components/pds-table/stories/pds-table.stories.js (2)
libs/core/src/components/pds-box/stories/pds-box.stories.js (1)
BaseTemplate(11-52)libs/core/src/components/pds-input/stories/pds-input.stories.js (1)
BaseTemplate(33-56)
libs/core/src/components/pds-table/pds-table-head/test/pds-table-head.spec.tsx (1)
libs/react/src/components/proxies.ts (1)
PdsTableHead(94-94)
🔇 Additional comments (17)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (1)
103-119: LGTM!Good negative test case verifying that the
has-dividerclass is not applied when therow-dividersprop is absent.libs/core/src/components/pds-table/pds-table.tsx (1)
41-45: LGTM!The
rowDividersprop is well-declared with appropriate defaults and reflection. The JSDoc clearly describes the behavior.libs/core/src/components/pds-table/docs/pds-table.mdx (2)
11-11: LGTM!Good documentation improvements that clarify component tag names and add a dedicated section for the table head with its properties.
Also applies to: 32-37, 40-40, 46-46, 52-52
220-299: LGTM!Excellent documentation example demonstrating the new
border,background, androwDividersprops with both React and web component syntax. The examples are complete and accurately showcase the combined visual effect.libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.scss (3)
2-2: Good semantic fix!Correcting the border color from
--pine-color-background-subtleto--pine-color-border-subtleimproves semantic clarity and ensures the design token is used for its intended purpose.
12-14: LGTM!Typography and padding adjustments align with the PR objective to standardize alignment between head cells and regular cells. The addition of
letter-spacingand the padding token update improve visual consistency.
31-39: LGTM!The conditional styling for
has-head-backgroundandhas-head-borderclasses correctly responds to parent table-head props. The comments clearly explain the purpose of each rule.libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.tsx (3)
62-71: LGTM! Good initialization approach.Initializing the state flags from parent attributes during
componentWillLoadensures the correct classes are applied on the first render.
101-107: LGTM! Proper cleanup implementation.The cleanup logic correctly disconnects the observer, preventing potential memory leaks.
204-210: LGTM! Class application follows existing patterns.The conditional class additions for background and border states are consistent with the component's existing styling approach.
libs/core/src/components/pds-table/pds-table-head/pds-table-head.tsx (1)
24-34: LGTM! Well-defined props with proper reflection.The new
borderandbackgroundprops are correctly configured withreflect: true, enabling the host attribute selectors in the SCSS to work as expected.libs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsx (1)
182-202: LGTM! Good test coverage for new styling attributes.These tests verify that the component correctly applies CSS classes when the parent
pds-table-headhas border or background attributes.libs/core/src/components/pds-table/pds-table-head/pds-table-head.scss (1)
23-39: LGTM! Clean styling implementation.The host attribute selectors correctly apply border and background styles to both the table head and its child cells, maintaining visual consistency. The use of CSS custom properties aligns with the existing design system patterns.
libs/core/src/components/pds-table/stories/pds-table.stories.js (3)
8-27: LGTM! Well-organized argTypes.The argTypes properly categorize
borderandbackgroundunder "Table Head" to clarify their scope, and include helpful descriptions for all new controls.
30-64: LGTM! Template correctly wired for new props.The
BaseTemplateproperly binds the new boolean props using lit-html's?syntax and passes them to the appropriate components (borderandbackgroundtopds-table-head,rowDividerstopds-table).
324-334: LGTM! New story variant demonstrates the feature.The
RowDividersstory provides a clear example of the row dividers feature in isolation.libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx (1)
98-110: Well-structured lifecycle handling for dynamic DOM scenarios.Using
connectedCallbackto set up observers handles the row re-attachment case (e.g., after sorting) correctly, sincecomponentDidLoadonly fires once. The cleanup indisconnectedCallbackprevents memory leaks. Good approach.
|
@coderabbitai addressed the minor issue. please perform another review |
|
@pixelflips, thank you for addressing the issue! I'll perform another review of the changes now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx:
- Around line 80-105: The test can be flaky because it asserts classList on
`rows` without first ensuring `rows` exists and without waiting for potential
MutationObserver updates; after creating the page with `newSpecPage(...)` add
`await page.waitForChanges()` before querying `rows`, then assert `rows` is
defined/non-empty (e.g., check `rows.length > 0`) before iterating and asserting
`has-divider` and `is-last-row` on each `PdsTableRow` instance.
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (1)
107-123: Assert the row exists before checking classes.
This avoids confusing failures if the DOM shape changes (and removes optional-chaining in expectations).Proposed tweak
it('does not render with has-divider class when table does not have rowDividers prop', async () => { const page = await newSpecPage({ components: [PdsTable, PdsTableBody, PdsTableRow], html: ` <pds-table> <pds-table-body> <pds-table-row> <pds-table-cell>Row 1</pds-table-cell> </pds-table-row> </pds-table-body> </pds-table> `, }); const row = page.root?.querySelector('pds-table-row'); - expect(row?.classList.contains('has-divider')).toBe(false); + expect(row).not.toBeNull(); + expect(row!.classList.contains('has-divider')).toBe(false); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (1)
libs/react/src/components/proxies.ts (3)
PdsTable(91-91)PdsTableBody(92-92)PdsTableRow(96-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-specs (22)
- GitHub Check: test-specs (20)
- GitHub Check: test-specs (18)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx
Show resolved
Hide resolved
|
@coderabbitai addressed minor issue. please review again. |
|
@pixelflips, thank you for addressing the issue! I'll perform another review of the changes now. ✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (1)
115-131: Consider adding waitForChanges() for consistency.The negative test case correctly verifies the absence of the
has-dividerclass. However, for consistency with the positive test case and to ensure any asynchronous updates have completed, consider addingawait page.waitForChanges()after the page setup.♻️ Suggested enhancement
}); + // Wait for any potential updates to complete + await page.waitForChanges(); + const row = page.root?.querySelector('pds-table-row'); expect(row?.classList.contains('has-divider')).toBe(false); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (1)
libs/react/src/components/proxies.ts (3)
PdsTable(91-91)PdsTableBody(92-92)PdsTableRow(96-96)
🔇 Additional comments (3)
libs/core/src/components/pds-table/pds-table-row/test/pds-table-row.spec.tsx (3)
34-34: LGTM! Correct type assertion for custom checkbox component.The change from
HTMLFormElementtoHTMLElementis appropriate for thepds-checkboxcustom element.Also applies to: 71-71
77-78: LGTM! Appropriate type assertion to access component properties.The cast to
HTMLPdsTableRowElementcorrectly enables access to theindeterminateproperty.
80-113: LGTM! Comprehensive test coverage for row dividers feature.The test properly verifies:
- All rows receive the
has-dividerclass whenrowDividersis enabled- The
is-last-rowclass is applied exclusively to the last row- Defensive checks ensure rows exist before assertions
- Appropriate wait for MutationObserver updates
2 participants
Description
This PR adds styling props to the table component to enable visual separation between the header and body, and between rows. The
borderandbackgroundprops onpds-table-headadd top/bottom borders and a subtle background color to the header. TherowDividersprop onpds-tableadds divider borders between rows (excluding the last row). Also fixes alignment issue between head cells and regular cells by standardizing padding and letter-spacing.Fixes DSS-65
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: