fix(pds-table-row): fix checkbox double-fire and z-index bleed-through#679
fix(pds-table-row): fix checkbox double-fire and z-index bleed-through#679pixelflips merged 3 commits intomainfrom
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe pds-table-row component replaces its click handler with 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.3.0)libs/core/src/components/pds-table/pds-table.scssConfigurationError: Could not find "stylelint-config-sass-guidelines". Do you need to install the package or use the "configBasedir" option? Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx (1)
43-43: Consider adding explicit type annotation for better type safety.The
eventparameter could be typed more precisely to improve code clarity and enable compile-time checks.💡 Suggested type annotation
- private handleCheckboxChange = (event: CustomEvent) => { + private handleCheckboxChange = (event: CustomEvent<{ checked: boolean }>) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx` at line 43, The parameter for handleCheckboxChange lacks a precise type; change its signature to accept a typed CustomEvent (e.g., event: CustomEvent<{ checked: boolean }>) so TypeScript can validate event.detail usage; update any internal references to use event.detail.checked and ensure the event payload shape matches the emitter that dispatches the checkbox change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/core/src/components/pds-table/pds-table-row/pds-table-row.tsx`:
- Line 43: The parameter for handleCheckboxChange lacks a precise type; change
its signature to accept a typed CustomEvent (e.g., event: CustomEvent<{ checked:
boolean }>) so TypeScript can validate event.detail usage; update any internal
references to use event.detail.checked and ensure the event payload shape
matches the emitter that dispatches the checkbox change.
77411d7 to
00d0e32
Compare
|
@coderabbitai i have addressed the nitpick comment, please review. |
|
✅ Actions performedReview triggered.
|
sdhull
left a comment
There was a problem hiding this comment.
looks promising, thank you! I played with the deployment preview and all seems well
Description
Fixes two issues with
pds-tableselectable tables, reported from Kajabi Products (Media Library index page with selectableDataTableComponent).1. Checkbox double-fire (
pds-table-row)The
pdsTableRowSelectedevent'sdetail.isSelectedproperty is unreliable. When clicking a checkbox, the event sometimes fires twice in the same synchronous tick — first withisSelected: true, then immediately withisSelected: false— even though the user only checked the box.Root cause: In
pds-table-row.tsx, the checkbox was wired withonClickon thepds-checkboxelement. Insidepds-checkbox's shadow DOM, a<label htmlFor="X">wraps<input id="X">. When a click lands on the<label>(not directly on the<input>), the browser's label activation algorithm dispatches a synthetic click on the input, causing theonClickhandler to fire twice. The handler toggledisSelectedvia!this.isSelected, so it toggled totruethen immediately back tofalse.This was intermittent because it depended on whether the click hit the native
<input>element directly (one event, correct) or the<label>area (two events, broken).Fix: Switch from
onClicktoonPdsCheckboxChangeon the internalpds-checkbox. ThepdsCheckboxChangeevent fires exactly once per interaction (from the native input'sonChange). Readsevent.detail.checkeddirectly instead of toggling with!this.isSelected.2. Z-index bleed-through (
pds-table)Sticky checkbox cells in
pds-table-rowandpds-table-headusez-index: var(--pine-z-index-raised)which resolves to1000. When a consuming application has a fixed header with a lower z-index (e.g.901), the sticky checkbox cells and toolbar elements render on top of the header when the page is scrolled.Fix: Add
isolation: isolateto thepds-table:hoststyles. This creates a stacking context that scopes all internal z-indices so they don't compete with page-level elements. Internal stacking (sticky cells above regular cells, fixed columns, scroll shadows) continues to work correctly within the table's own stacking context.Fixes DSS-168
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: