Skip to content

data-component adr part 2#7776

Open
llastflowers wants to merge 47 commits intomainfrom
llastflowers/6497/data-component-ADR-part-2
Open

data-component adr part 2#7776
llastflowers wants to merge 47 commits intomainfrom
llastflowers/6497/data-component-ADR-part-2

Conversation

@llastflowers
Copy link
Copy Markdown
Contributor

@llastflowers llastflowers commented Apr 23, 2026

Relates to https://github.com/github/primer/issues/6497

Changelog

New

Add data-component attributes and associated tests for Blankslate, BranchName, Breadcrumbs, ButtonGroup, Checkbox, CheckboxGroup, CircleBadge, ConfirmationDialog, and CounterLabel.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 23, 2026

🦋 Changeset detected

Latest commit: b68573d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Apr 23, 2026
@github-actions github-actions Bot requested a deployment to storybook-preview-7776 April 23, 2026 23:45 Abandoned
@github-actions github-actions Bot temporarily deployed to storybook-preview-7776 May 6, 2026 18:00 Inactive
@github-actions github-actions Bot temporarily deployed to storybook-preview-7776 May 6, 2026 19:03 Inactive
@llastflowers llastflowers added Canary Release Apply this label when you want CI to create a canary release of the current PR and removed Canary Release Apply this label when you want CI to create a canary release of the current PR labels May 6, 2026
…data-component to Dialog.tsx while I'm at it
@github-actions github-actions Bot requested a deployment to storybook-preview-7776 May 6, 2026 23:20 Abandoned
@llastflowers llastflowers added Canary Release Apply this label when you want CI to create a canary release of the current PR and removed Canary Release Apply this label when you want CI to create a canary release of the current PR labels May 6, 2026
@github-actions github-actions Bot temporarily deployed to storybook-preview-7776 May 6, 2026 23:30 Inactive
@primer-integration
Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/20172

@primer-integration
Copy link
Copy Markdown

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

@llastflowers llastflowers changed the title Llastflowers/6497/data component adr part 2 data-component adr part 2 May 7, 2026
'aria-label'?: string
}

const BreadcrumbsMenuItem = React.forwardRef<HTMLDetailsElement, BreadcrumbsMenuItemProps>(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should this element have a data-component as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure; it's not exported so I didn't think so. Do you think it should?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I vote we add it, your call though!

Comment thread packages/react/src/Breadcrumbs/Breadcrumbs.tsx
Comment on lines 80 to 81
Label: CheckboxOrRadioGroupLabel,
Validation: CheckboxOrRadioGroupValidation,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should Label, Validation have data-component as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure! Would you suggest including Caption too in that case?

These all have to pass through CheckboxOrRadioGroup etc. too, which are also used by RadioGroup and its sub-components. Let me know what pattern you think would be best for that so [data-component="CheckboxGroup"] and [data-component="RadioGroup"] still work separately.

For this PR, I wonder if it would be better to just take CheckboxGroup out for now until I can open a new one with RadioGroup too? I don't really want to make this one even bigger 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, Caption too!. I think similar to the way you did CheckboxOrRadioGroup, maybe take the parentName as the prop?

// @ts-expect-error inputProp needs a non nullable ref
<input
{...inputProps}
data-component={dataComponent ?? 'Checkbox'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why does this need to take in dataComponent as a prop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make sure [data-component=FilteredActionList.SelectAllCheckbox] (and possibly others that we haven't run into yet) could still be used without being overridden by [data-component=Checkbox]. Let me know if you have a different suggestion!

<span {...counterProps} className={clsx(className, classes.CounterLabel)}>
<span
{...counterProps}
data-component={dataComponent ?? 'CounterLabel'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why does this need to take in dataComponent as a prop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make sure [data-component=ButtonCounter] could still be used without being overridden by [data-component=CounterLabel]. Let me know if you have a different suggestion!

Comment thread packages/react/src/Dialog/Dialog.tsx Outdated
Comment thread packages/react/src/Dialog/Dialog.tsx Outdated
Comment thread packages/react/src/Dialog/Dialog.tsx
Comment thread packages/react/src/Dialog/Dialog.test.tsx
@llastflowers llastflowers requested a review from francinelucca May 8, 2026 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants