fix(pds-button): fix empty icon slot wrapper spacing#678
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
bug
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe pds-button component’s automatic pseudo-class-based icon-hiding was replaced with an explicit 🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/core/src/components/pds-button/pds-button.tsx (1)
160-166: Consider usingassignedNodes({ flatten: true })for nested slot handling.The current implementation uses
assignedNodes()without options, which won't include nodes from nested slots if the slotted content itself contains slots. While this is likely fine for the typical use case (slottingpds-iconelements), using{ flatten: true }would be more robust.Additionally,
assignedNodes()includes text nodes (including whitespace), which could cause false positives. If only elements should be considered,assignedElements()might be more appropriate.💡 Optional: Use assignedElements() for stricter element-only detection
private handleStartSlotChange = (event: Event) => { - this.hasStartContent = (event.target as HTMLSlotElement).assignedNodes().length > 0; + this.hasStartContent = (event.target as HTMLSlotElement).assignedElements().length > 0; }; private handleEndSlotChange = (event: Event) => { - this.hasEndContent = (event.target as HTMLSlotElement).assignedNodes().length > 0; + this.hasEndContent = (event.target as HTMLSlotElement).assignedElements().length > 0; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/components/pds-button/pds-button.tsx` around lines 160 - 166, The slot-change handlers handleStartSlotChange and handleEndSlotChange should use flattened assignment and/or element-only detection to avoid nested slots and text-node false positives; change the checks so they call event.target.assignedElements({ flatten: true }) (or assignedNodes({ flatten: true }) filtered for nodeType === Node.ELEMENT_NODE) and set hasStartContent/hasEndContent based on the resulting element list length > 0.libs/core/src/components/pds-button/test/pds-button.spec.tsx (1)
237-247: Snapshot shows--emptyclass despite content being slotted.This test renders a button with
<pds-icon slot="start">slotted, but the snapshot still showspds-button__icon--emptyon the start wrapper. This appears inconsistent with the expected behavior—when content is slotted, the--emptyclass should be removed.This is likely because
newSpecPagedoesn't automatically fireslotchangeevents, sohasStartContentremainsfalse. While the behavior is technically correct for the test framework's limitations, the snapshot doesn't reflect real-world runtime behavior where the class would be absent.Consider adding a comment explaining this limitation, or restructuring the test to simulate the
slotchangeevent (as done in the new "slot empty class" test suite) to produce a more accurate snapshot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/components/pds-button/test/pds-button.spec.tsx` around lines 237 - 247, The snapshot shows pds-button__icon--empty because newSpecPage doesn't fire slotchange so hasStartContent remains false; to fix, update the test in pds-button.spec.tsx to simulate a slotchange for the start slot (as done in the "slot empty class" suite) after rendering so the component updates and removes pds-button__icon--empty, or alternatively add a short comment explaining this test-framework limitation; locate the render using newSpecPage and the component's slot handling (hasStartContent / slotchange) and either dispatch a slotchange event and await changes or document why the snapshot intentionally shows the --empty class.
🤖 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-button/pds-button.tsx`:
- Around line 160-166: The slot-change handlers handleStartSlotChange and
handleEndSlotChange should use flattened assignment and/or element-only
detection to avoid nested slots and text-node false positives; change the checks
so they call event.target.assignedElements({ flatten: true }) (or
assignedNodes({ flatten: true }) filtered for nodeType === Node.ELEMENT_NODE)
and set hasStartContent/hasEndContent based on the resulting element list length
> 0.
In `@libs/core/src/components/pds-button/test/pds-button.spec.tsx`:
- Around line 237-247: The snapshot shows pds-button__icon--empty because
newSpecPage doesn't fire slotchange so hasStartContent remains false; to fix,
update the test in pds-button.spec.tsx to simulate a slotchange for the start
slot (as done in the "slot empty class" suite) after rendering so the component
updates and removes pds-button__icon--empty, or alternatively add a short
comment explaining this test-framework limitation; locate the render using
newSpecPage and the component's slot handling (hasStartContent / slotchange) and
either dispatch a slotchange event and await changes or document why the
snapshot intentionally shows the --empty class.
|
@coderabbitai i addressed the nitpick comments, review again. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/core/src/components/pds-button/pds-button.tsx (1)
102-103: Initialize slot state on load to avoid first-render “empty” flicker.
hasStartContent/hasEndContentonly update onslotchange. If content is present at initial render and the event is delayed or not fired in a target browser, the icon wrapper can stay hidden or flash. Consider setting initial state from light‑DOM slot nodes and keepslotchangefor updates. Also confirmassignedElements({ flatten: true })support in all target browsers (especially Safari versions in your support matrix).♻️ Example initialization (before first render)
export class PdsButton { `@Element`() el: HTMLPdsButtonElement; @@ `@State`() hasStartContent = false; `@State`() hasEndContent = false; + + componentWillLoad() { + this.hasStartContent = !!this.el.querySelector('[slot="start"]'); + this.hasEndContent = !!this.el.querySelector('[slot="end"]'); + }Also applies to: 160-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/core/src/components/pds-button/pds-button.tsx` around lines 102 - 103, Initialize hasStartContent/hasEndContent from the light DOM before first render in the PdsButton component (e.g., in componentWillLoad or connectedCallback) by querying the named slots (slot[name="start"] and slot[name="end"]) and using slot.assignedElements({ flatten: true }) to detect element children and fall back to slot.assignedNodes() filtered for nodeType === Node.ELEMENT_NODE if assignedElements isn't available; keep the existing slotchange handlers to update state on changes and set hasStartContent/hasEndContent accordingly so icons don't flicker on initial render, and consider adding a short comment about browser support for assignedElements usage and the fallback.
🤖 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-button/pds-button.tsx`:
- Around line 102-103: Initialize hasStartContent/hasEndContent from the light
DOM before first render in the PdsButton component (e.g., in componentWillLoad
or connectedCallback) by querying the named slots (slot[name="start"] and
slot[name="end"]) and using slot.assignedElements({ flatten: true }) to detect
element children and fall back to slot.assignedNodes() filtered for nodeType ===
Node.ELEMENT_NODE if assignedElements isn't available; keep the existing
slotchange handlers to update state on changes and set
hasStartContent/hasEndContent accordingly so icons don't flicker on initial
render, and consider adding a short comment about browser support for
assignedElements usage and the fallback.
|
@coderabbitai in regards to your latest nitpick comment: The shadow DOM elements don't exist until after Stencil's first render() call, so we can't call slot.assignedElements() in componentWillLoad or connectedCallback — there's nothing to query yet. The alternative (querying light DOM with querySelector('[slot="start"]')) is exactly the synchronous DOM check that PR #668 replaced due to race conditions withStencil's async task queue. Re-adding it would reintroduce the original bug (DSS-147). The current approach defaults to hidden (--empty) and lets slotchange toggle visibility. This is the safe direction — slotchange fires during slot distribution before paint, so there's no visible flicker. A assignedElements fallback is also unnecessary since it shares the same browser support floor as shadow DOM itself. |
|
Tip For best results, initiate chat on the files or code changes.
Your approach is the correct solution given Stencil's rendering lifecycle and the constraints. I appreciate you taking the time to clarify the technical rationale! |
2 participants
Description
Fixes extra padding/spacing on buttons when no icon is slotted into the start/end slots.
PR #668 fixed a race condition by always rendering the
<slot name="start">and<slot name="end">wrappers, using CSS:has(::slotted(*))to hide them when empty. However,:has(::slotted(*))has unreliable cross-browser support (particularly Safari). When the selector doesn't match, the empty.pds-button__iconspans remaindisplay: inline-flex, and the parent'sgapcreates visible spacing where none should exist.This replaces the CSS
:has()approach withslotchangeevent handlers that toggle a--emptyCSS class, following the existing pattern used inpds-input.Fixes DSS-167
Screenshots
Type of change
How Has This Been Tested?
Checklist: