Skip to content

feat(pds-box): add tag prop for semantic HTML elements#767

Closed
QuintonJason wants to merge 9 commits into
mainfrom
feat/pds-box-tag-prop
Closed

feat(pds-box): add tag prop for semantic HTML elements#767
QuintonJason wants to merge 9 commits into
mainfrom
feat/pds-box-tag-prop

Conversation

@QuintonJason

@QuintonJason QuintonJason commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a tag prop to pds-box so it can render as semantic HTML elements (main, section, article, header, footer, nav, aside)
  • Default div behavior is unchanged — styles and classes stay on the host for backward compatibility
  • Non-div tags render an inner semantic element with display: contents on the host so grid/row layout continues to work
  • Storybook control and spec coverage added for all supported tag values

Test plan

  • unit tests (pds-box.spec.tsx — tag prop coverage for all semantic variants)
  • e2e tests
  • accessibility tests
  • tested manually
  • other:

Test Configuration:

  • Pine versions: local branch
  • OS: macOS
  • Browsers:
  • Screen readers:
  • Misc: npx nx run @pine-ds/core:test --testPathPattern=pds-box.spec passes

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes
  • Design has QA'ed and approved this PR

Note

Medium Risk
Render tree and ARIA handling change for non-div tags and row layout selectors were extended; default div usage is unchanged but semantic/landmark usage needs correct labeling and regression checks in real pages.

Overview
Adds a tag prop to pds-box so consumers can render main, section, nav, and other allowed semantic elements instead of always using the host as the flex/grid box. tag="div" (default) keeps the prior DOM: layout classes and inline styles stay on the Host.

For non-div tags, the component renders an inner semantic wrapper with the existing pds-box classes and styles, sets the host to display: contents via pds-host--contents, and moves role / aria-* from the host onto that inner element (with a MutationObserver so runtime ARIA updates still apply). Invalid tag values warn and fall back to div.

pds-box.scss row/column rules now target .pds-row > pds-box > [class*='pds-box-'] so grid sizing still works when the styled node is inside the custom element. Shared utils gain BoxTagType, isBoxTag, and readAriaAttributes; Storybook, MDX, and unit specs cover semantic rendering and ARIA behavior.

Reviewed by Cursor Bugbot for commit 41fc91d. Bugbot is set up for automated code reviews on this repo. Configure here.

@QuintonJason QuintonJason added the ran-gauntlet Multi-agent review gauntlet has been run on this branch label Jun 15, 2026
@netlify

netlify Bot commented Jun 15, 2026

Copy link
Copy Markdown

Deploy Preview for pine-design-system ready!

Name Link
🔨 Latest commit 41fc91d
🔍 Latest deploy log https://app.netlify.com/projects/pine-design-system/deploys/6a3470d7ad1e580008a115fe
😎 Deploy Preview https://deploy-preview-767--pine-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions github-actions Bot added the package: core Changes have been made to the Core package label Jun 15, 2026
Comment thread libs/core/src/components/pds-box/pds-box.scss
Comment thread libs/core/src/components/pds-box/pds-box.tsx
Comment thread libs/core/src/components/pds-box/pds-box.tsx
@QuintonJason QuintonJason force-pushed the feat/pds-box-tag-prop branch from 474256e to 99efa12 Compare June 18, 2026 22:06
@QuintonJason QuintonJason requested a review from pixelflips June 18, 2026 22:07
Comment thread libs/core/src/components/pds-box/pds-box.tsx
Comment thread libs/core/src/components/pds-box/pds-box.tsx
Comment thread libs/core/src/components/pds-box/pds-box.tsx

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 90912b1. Configure here.

Comment thread libs/core/src/components/pds-box/pds-box.tsx
@QuintonJason QuintonJason self-assigned this Jun 18, 2026
@pixelflips

pixelflips commented Jun 18, 2026

Copy link
Copy Markdown
Member

@QuintonJason, taking a look at this and want to make sure I'm not missing context before I land on a verdict.

My gut says authors could just wrap pds-box in the semantic element they need (<nav><pds-box>...</pds-box></nav>) rather than swapping the rendered tag inside the shadowDOM. Semantic elements in the light DOM have better AT/screen reader support than those inside shadow DOM, so the ARIA forwarding here is kind of working around a problem the approach itself introduced. It also pulled in a fair bit of complexity. The display: contents on the host, the ARIA forwarding, and extra CSS selectors to handle the shifted DOM layer. Just using the wrapping would skip that entirely.

The only case where tag wins over wrapping is if you specifically need the box styles (padding, gap, direction, etc.) on the semantic element itself and not a child of it. I feel like I may be missing something here, so just let me know.

@pixelflips

Copy link
Copy Markdown
Member

@QuintonJason, I looked closer, and pds-box doesn't actually use shadow: true, so the semantic element lands in the light DOM either way. That part of my concern doesn't hold up. Still not sure, compared to just using the actual elements, so let me know your thoughts here.

@QuintonJason

Copy link
Copy Markdown
Contributor Author

Closing this. After review, feedback and thinking through maintenance cost, the tag prop adds too much ongoing complexity (display:contents, ARIA forwarding, dual render paths) for the use case. Wrapping pds-box in semantic HTML covers our needs without that debt. Happy to add a short docs note on when to wrap instead if that would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core Changes have been made to the Core package ran-gauntlet Multi-agent review gauntlet has been run on this branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants