ADR-023: Public data-component API for targeting component parts#7576
ADR-023: Public data-component API for targeting component parts#7576lukasoppermann wants to merge 5 commits intomainfrom
Conversation
Establishes data-component as a public, stable API with a consistent naming convention using dot notation mirroring the React component API. This enables consumers to reliably target internal parts of components for style overrides and JS queries despite CSS Module hash changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
There was a problem hiding this comment.
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
|
🤖 Formatting issues have been automatically fixed and committed to this PR. |
siddharthkp
left a comment
There was a problem hiding this comment.
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"] {}| | -------------------- | ------------------------------------------------ | | ||
| | `ComponentName.Part` | `ActionList.Description`, `ActionList.Selection` | | ||
| | `PREFIX_Part` | `PH_LeadingAction`, `PH_Title`, `PH_Navigation` | | ||
| | `camelCase` | `buttonContent`, `leadingVisual`, `text` | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No, I think this is not needed.
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
left a comment
There was a problem hiding this comment.
Approving! Left some comments.
| <li data-component="ActionList.Item"></li> | ||
| ``` | ||
|
|
||
| 3. **Internal structural parts** (DOM elements that are not exposed as a |
There was a problem hiding this comment.
How do we determine which internal parts are meaningful enough to have a data-component attribute?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is there any harm in over-doing it?
There was a problem hiding this comment.
Is there any harm in over-doing it?
My take as well! let's do all the things
| impossible for consumers to reliably target internal parts of a component for | ||
| style overrides using class selectors. | ||
|
|
||
| Consumers need the ability to target component parts for legitimate use cases: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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;
}
}There was a problem hiding this comment.
@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" />} />There was a problem hiding this comment.
@siddharthkp
They are trying to replicate the new filter UI style.
For your example, yes, this of course is nice.
I have two issues with that:
- We don't have this for all our components, and it seems like this will not be a quick fix.
- This means that if I want to just change the text color for something, or icon color, I always need to add nested components for everything, this may be a bit of on overkill for small changes.
However, I think 2 is a matter of opinion and I am happy to shift there, but I see 1 as a problem.
Do you see this differently?
Another example I remember is a case where folks needed to change the color of the icon in the banner, which I think is not easily possible.
There was a problem hiding this comment.
Do you see this differently?
Nope, same page as you
- We don't have this for all our components, and it seems like this will not be a quick fix.
Yes, absolutely. I was trying to point the ideal scenario of how it should be.
- This means that if I want to just change the text color for something, or icon color, I always need to add nested components for everything, this may be a bit of on overkill for small changes.
Maybe but still very little additional work, so for customisation that goes off the system, I don't feel too bad about it.
There was a problem hiding this comment.
Another example use cases I learned yesterday. Dustin needed to adjust the Dialog position as he was using it to build the command palette. He only made it work with a css hack, as the class is not forwarded to the needed element.
He said that having a way to at least temporary move on (and possibly report a bug / feature request to primer) would have helped him a lot.
|
|
||
| ### Negative | ||
|
|
||
| - **Breaking change for existing consumers.** Anyone currently relying on the |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
72 files in github/github-ui, not all of them are primer components, but I'm assuming most of them are
There was a problem hiding this comment.
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 🦾
|
|
||
| ### Migration | ||
|
|
||
| Existing `data-component` values must be migrated to the new convention. This |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is an interesting idea. I'd be open to it. Wonder what other folks think @siddharthkp @TylerJDev ?
There was a problem hiding this comment.
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.
| impossible for consumers to reliably target internal parts of a component for | ||
| style overrides using class selectors. | ||
|
|
||
| Consumers need the ability to target component parts for legitimate use cases: |
There was a problem hiding this comment.
+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.
| **Why not chosen:** Only works with Shadow DOM, which React does not use. Not | ||
| applicable to our architecture. | ||
|
|
||
| ### 3. Do nothing — keep `data-component` as an internal implementation detail |
There was a problem hiding this comment.
Can we look into why they are doing that? I think we should categorize and solve those use cases properly instead.
There was a problem hiding this comment.
I think this is a two step process:
- Teams adjust / experiment using data attribute selectors
- 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.
There was a problem hiding this comment.
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.
|
Some systems, like shd/cdn use I think this is nice, as you can easily nest it like What are your opinions? @hectahertz @siddharthkp @joshfarrant @TylerJDev @francinelucca? |
| **Why not chosen:** Only works with Shadow DOM, which React does not use. Not | ||
| applicable to our architecture. | ||
|
|
||
| ### 3. Do nothing — keep `data-component` as an internal implementation detail |
There was a problem hiding this comment.
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.
data-component identifies root elements of components and sub-components. data-slot identifies inner structural parts, scoped to their parent component. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
e54c9ff to
a5adc10
Compare
|
I now updated the ADR to include |
Honestly, not a fan of having both. Whether something is a data-component because it composes another primer component or is a data-slot because its local to the component (for now) is an implementation detail that should not matter to a consumer. Pick one or the other for simplicity. |
@siddharthkp if we just add When we have subcomponents, like |
Correct. Unless, the ActionList is rendered by SelectPanel, then you don't have access to it, because the SelectPanel does not let you compose :(
Correct.
If it's all the same, I'd like to keep the name |
|
Approving not to block this if you want to merge it, even though I still have the same concerns. Please consider my previous comments and how you'll maintain semver if this gets implemented. This will be hard to undo if consumers start using it. |
Adds ADR-023 proposing
data-componentas 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:
ActionList.Item,Button.LeadingVisual)data-variant,data-size) remain separate