ADR-023: Public data-component API for targeting component parts

[lukasoppermann]

Adds ADR-023 proposing data-component as a public, stable API for identifying component parts in the DOM. This gives consumers reliable selectors to target internal parts of components for style overrides and JS queries, independent of CSS Module hash changes.

Key decisions:

• Dot notation mirroring the React API (ActionList.Item, Button.LeadingVisual)
• State/modifier attrs (data-variant, data-size) remain separate
• Includes migration table for existing inconsistent values
• Complements CSS Layers (ADR-021) for a complete override mechanism

[changeset-bot]

⚠️ No Changeset found

Latest commit: e54c9ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[Copilot]

Pull request overview

This PR introduces ADR-023, which formalizes data-component as a public, stable API for identifying component parts in the DOM. The ADR addresses the current problem where CSS Modules generate unstable hashed class names, making it impossible for consumers to reliably target internal component parts for style overrides or JavaScript queries. By establishing a consistent naming convention using dot notation that mirrors the React component API (e.g., ActionList.Item, Button.LeadingVisual), this ADR provides consumers with a reliable mechanism to target component parts.

Changes:

• Proposes standardized dot notation naming convention (ComponentName.PartName) for data-component attributes
• Documents migration path for existing inconsistent data-component values across multiple components
• Establishes testing requirements and coverage guidelines for the data-component API

[primer]

🤖 Formatting issues have been automatically fixed and committed to this PR.

[siddharthkp]

I personally really like the idea (i think I introduced it to the codebase), so I approve!

target internal parts of a component for style overrides using class selectors.

I know you did not say anything against this but I think this part is worth highlighting more boldly in the ADR because the preferred/superior method is still to expose smaller parts and let people add their own classes to them.

<ActionList.Item className={classes.myCustomList} />
  <ActionList.Description>...</ActionList.Description>
</ActionList.Item>

.myCustomList {} 
.myCustomList [data-component="ActionList.Description"] {}

but it's better to do this:

<ActionList.Item className={classes.myCustomList} />
  <ActionList.Description className={classes.myCustomListDescription}..</ActionList.Description>
</ActionList.Item>

.myCustomList {}
.myCustomListDescription {}

Maybe a better example to showcase in the ADR would be a component where we don't let you compose

<SelectPanel items={...} className={classes.myCustomPanel />

.myCustomPanel [data-component="ActionList.Description"] {}

[siddharthkp]

ouch, i think i know why this is so.

ActionList.Description is a component that is part of the public API, so we expose that exactly.

But Button's leadingVisual is not part of the API so it ended up naming it after the className or internal variable name which is camelCase. I don't know if we need to differentiate them based on the nuance of public vs private, i don't think anyone else would care about it.

PH_LeadingAction, similar reason, but underscore is random, call it PageHeader.LeadingAction

[lukasoppermann]

No, I think this is not needed.

[lukasoppermann]

I personally really like the idea (i think I introduced it to the codebase), so I approve!

target internal parts of a component for style overrides using class selectors.

I know you did not say anything against this but I think this part is worth highlighting more boldly in the ADR because

Agree. I think this is a quick solution, that may still be helpful if you want to do small styling changes for all actionList.items or something.

But creating more composable subcomponents is definitely an additional need.

@siddharthkp feel free to adjust with a better example if you want.

[TylerJDev]

Approving! Left some comments.

[TylerJDev]

How do we determine which internal parts are meaningful enough to have a data-component attribute?

[lukasoppermann]

Good question, any ideas?

I think one aspect could be that they are functional, so not simple structure or a11y elements.

Do you think we will run into issues here or should we maybe just try it and see if it just works out? We could try to compile a list of what might make a meaningful element if you think this is helpful.

[siddharthkp]

Is there any harm in over-doing it?

[francinelucca]

Is there any harm in over-doing it?

My take as well! let's do all the things

[TylerJDev]

I think this is a good change! Is this something consumers are currently asking for, or are we anticipating future needs and implementing it in advance?

[hectahertz]

+1000% to @TylerJDev comment, I'd please like to see real problems and use cases for this that justify this.

See this https://github.com/github/primer/discussions/6393 for why I'm suggesting changes. I think this is unnecessary and could lead to a very fragile system.

[siddharthkp]

From slack, @hectahertz
Why is this a problem? do we want people changing internals outside of the api? Free reign?

I think this is slightly tangential to your post. The root of the problem is some components not being composable at all. For example, SelectPanel and Autocomplete are black boxes that take title, items, etc. but gives you little control over customising the styles of internal components

@hectahertz: What's the plan for us restyling those classes? are we going to have to make major releases anytime we change the styling of one of them or the dom structure as they will be breaking changes?

This is a tricky spot we always find ourselves in. If we don't give folks the ability to customise, the only option is to fork/eject. If we do give some customisations, we are now on the hook to support backward compatibility with those customisations as well. 😢

For me, a balance does not exist in the long term, because it's not uncommon for us to need to change the internal implementation details for accessibility or other fixes. I'd love to hear ideas on how we can solve for this

[lukasoppermann]

Yes, I am currently doing interviews with folks and have hear this as an issue a few times.

I don't have a list of the specific things, but I think especially copilot like copilot chat, ran into a lot of issues where they wanted to change small things but couldn't easily and thus rebuilt the component.

@hectahertz are you suggesting that adding data-attr will have a huge performance impact or are you saying teams can kill performance by messing with the components? Can you elaborate on why this would make the system fragile?

[lukasoppermann]

So enterprise has some cases where they are trying to style components different. Currently they are wrapping the react component into another react component to add styles. However, it is hard to target specific parts. The span is actually the trailing action part they want to style.

.chipInput {
  border-radius: var(--borderRadius-full) !important;

  & input:placeholder-shown + span {
    display: none;
  }
}

.chipInput span:last-child {
  color: var(--control-fgColor-disabled) !important;

  &:hover {
    color: var(--fgColor-muted) !important;
    cursor: pointer;
  }
}

[siddharthkp]

@lukasoppermann sorry, what component is that? Trying to see if there's a better way that already exists for this specific case

For example, with TextInput, the ideal solution that @hectahertz suggested already works:

<TextInput trailingVisual={() => <CalendarIcon className="custom-selector" />} />

and then you can target it

. & input:placeholder-shown + .custom-selector {}

I feel this is better than

. & input:placeholder-shown + [data-component=TextInput.TrailingVisual] {}

because it also let's you do other things like

<TextInput trailingVisual={() => <CalendarIcon data-testid="id" />} />

[TylerJDev]

I wonder if we have data on how many data-component attributes are used in CSS/DOM queries today. Either way, I don't anticipate a difficult migration.

[siddharthkp]

72 files in github/github-ui, not all of them are primer components, but I'm assuming most of them are

[joshfarrant]

Sounds great!

Stable, predictable component identifiers in the DOM could also be useful for AI agents too. An agent inspecting rendered output could use data-component values to understand the component hierarchy without needing access to source code, which is a nice added benefit 🦾

[joshfarrant]

If we're formalising a data attribute for component identification, do you think there's any benefit in namespacing the data attribute? E.g.,

<div data-primer-component="Button.Content">

It seems unlikely that there would ever be a collision with other libraries/projects as I can't name any which use data-component, but I wonder whether you'd considered namespacing it as part of the formalisation of the attribute?

It could also help ease the transition too, as introducing a new attribute (data-primer-component) rather than renaming values on the existing data-component would allow both to coexist during a deprecation period. Consumers could migrate their selectors incrementally rather than having all existing data-component references break at once.

[lukasoppermann]

This is an interesting idea. I'd be open to it. Wonder what other folks think @siddharthkp @TylerJDev ?

[siddharthkp]

eh, i don't feel strongly about it. I like the simplicity of data-component, I'd rather keep it.

Consumers could migrate their selectors incrementally rather than having all existing data-component references break at once.

Depends on how many we have. It's easy to target both old and new values in a css selector, so we can migrate without a breaking change.

[hectahertz]

+1000% to @TylerJDev comment, I'd please like to see real problems and use cases for this that justify this.

See this https://github.com/github/primer/discussions/6393 for why I'm suggesting changes. I think this is unnecessary and could lead to a very fragile system.

[hectahertz]

Can we look into why they are doing that? I think we should categorize and solve those use cases properly instead.

[lukasoppermann]

I think this is a two step process:

1. Teams adjust / experiment using data attribute selectors
2. Primer team upstreams proven customizations to the system

The problem is that we can't deliver at feature team speed and that the teams need ways to customize components to explore ideas e.g. in flagged ships.

If they always have to rebuild everything, it is unlikely they adjust it to use primer and create a PR once they have proven their idea is needed, because for them it brings little benefit compared to focusing on a new feature.

[francinelucca]

I like the idea of this , cuts down on teams having to wait for Primer to deliver on small customization/changes. We can then upstream and cleanup without teams having to be blocked for a week waiting on a Primer release.

[lukasoppermann]

Some systems, like shd/cdn use data-slot e.g. here. This allows for a combination of data-component on the actual component e.g. ActionMenu.Button and data-slot on the sub-elements like like TrailingAction (this actually uses a data-component at the moment).

I think this is nice, as you can easily nest it like [data-component="Button"] [data-slot="icon"], but I am not sure if it is a great idea.

What are your opinions? @hectahertz @siddharthkp @joshfarrant @TylerJDev @francinelucca?

[francinelucca]

✨

[francinelucca]

I like the idea of this , cuts down on teams having to wait for Primer to deliver on small customization/changes. We can then upstream and cleanup without teams having to be blocked for a week waiting on a Primer release.