Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADR] Add prefix to design tokens #1104

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[ADR] Add prefix to design tokens #1104

wants to merge 4 commits into from

Conversation

lukasoppermann
Copy link
Contributor

@lukasoppermann lukasoppermann commented Dec 5, 2024

Summary

This ADR discusses if we should add a prefix to primer primitives.

Please comment in the file or this PR as a comment.

@lukasoppermann lukasoppermann requested review from a team as code owners December 5, 2024 14:16
Copy link

changeset-bot bot commented Dec 5, 2024

⚠️ No Changeset found

Latest commit: 1eba2b6

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


The following decisions need ot be made:

1. Do we think that this is a legit problem that is serious enough to invest somewhat significant resources into?
Copy link
Contributor

Choose a reason for hiding this comment

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

No

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed prefix at length when we first introduced our new set of tokens. I'm sure I could dig it up 😄

I don't think we need a prefix for product tokens, especially because we do use a prefix for brand which is where collisions are actually very likely to happen.

If we're worried about people defining new tokens with the same names as our tokens, we can lint for that and not allow it using stylelint or eslint.

We simply don't have the resources right now to invest in a mass rename like we've done earlier this year, nor do I think it's a good idea.

Copy link
Contributor Author

@lukasoppermann lukasoppermann Dec 6, 2024

Choose a reason for hiding this comment

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

We discussed prefix at length when we first introduced our new set of tokens. I'm sure I could dig it up 😄

If there are more good arguments for and against it, that would be great.

If we're worried about people defining new tokens with the same names as our tokens, we can lint for that and not allow it using stylelint or eslint.

I think this is the main problem.

I do agree with the resources. However, the topic came up again and I want to have our decision documented, hence this ADR.

@langermank could you quickly outline how a linting against this would work, so that we can add it as an alternative to the ADR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share where this is coming up? I haven't personally seen it discussed or encountered open issues so I'm curious 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came up in a conversation I had Reza.

I think if we have no open issues and since there is a huge amount of work required it would be fair to keep it as is.
However, we should document this decision in an ADR. Thats exactly what they are for, right?

Copy link
Contributor

@rezrah rezrah Dec 9, 2024

Choose a reason for hiding this comment

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

👋 Sorry I'm late here. For context, I flagged this as @lukasoppermann shared an example of the upcoming component-level variable format which are already prefixed for Brand, but won't be for product.

Extrapolating from that example:

Primer Brand: --brand-headerSearch-bgColor
Primer Product: --headerSearch-bgColor
Feature teams: --headerSearch-customThing-bgColor

My anecdotal thoughts on the above were:

  • It's very easy to distinguish (in source code and debugging tools) between Primer Brand-owned component tokens from those custom-implemented by feature teams due to the prefix
  • It's not so easy to do the same with Primer product-specific tokens for component-specific variables
  • Component tokens are (IMO) different to low-level primitives like fgColor because they contrastingly lead to an exponential increase in CSS variables creation. With the move to CSS modules, feature teams will inevitably create many new variables, which could exacerbate the opportunities for collision and confusion.

This is pretty speculative, and largely based on my recent experience working on a feature team, where we created many component-level CSS variables.

It's one of those can-of-worms that easier to 'contain' now rather than later, as you could prefix component-scoped tokens in Primer.

If we're worried about people defining new tokens with the same names as our tokens, we can lint for that and not allow it using stylelint or eslint.

👍 That's fair, though I think the ownership ambiguity and indirection would still be there. It also takes agency away from feature teams to author their own arbitrary tokens, when its relatively easy for Primer to prefix (see above - prefixing component tokens only).

We discussed prefix at length when we first introduced our new set of tokens. I'm sure I could dig it up 😄

Nice. Sorry for drudging up an old conversation. This arose from a conversation I had with @lukasoppermann, but will defer to y'all on if/how you wanna address it. We're also not blocked by this on the Brand side as we have the prefix in place today ✌️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also takes agency away from feature teams to author their own arbitrary tokens, when its relatively easy for Primer to prefix (see above - prefixing component tokens only).

The problem is that prefix is easy on the "add to primitives" side, but not quite as easy on the "make sure no outdated tokens are used in github" side.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @rezrah! It sounds like you're referring specifically to component level tokens which I think is interesting to consider. When we shipped tokens, we included the option for a prefix specifically for Brand, but we also had a primer prefix originally in the first set. We later decided to remove it as it felt redundant. I can see how this is shifting with the move towards CSS Modules.

The naming convention allows for prefix and also namespace which we only use for base tokens right now. If we were to explore a "primer" namespace for component/pattern tokens, that could work for both Product and Brand UI:

--pc-control-bgColor-rest
--brand-pc-control-bgColor-rest

Using pc as Primer Component but idk 😄

So this model excludes functional tokens because this is not an area teams should be adding to or customizing. --fgColor-default is what it is and it would be redundant for teams to be introducing new tokens for established patterns. I do understand why defining tokens for components is useful, though.

I believe in the original naming convention conversations, we felt that the component/pattern name itself was unique enough to not need a prefix. But I'm not sure that's true anymore.


1. Do we think that this is a legit problem that is serious enough to invest somewhat significant resources into?
2. If yes, what is an appropriate prefix?
3. If no, do we need some kind of guidance in primer.style, or are we good without it?
Copy link
Contributor

Choose a reason for hiding this comment

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

What would the guidance be? I don't think CSS variable collisions are specific enough to Primer to need documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would the guidance be? I don't think CSS variable collisions are specific enough to Primer to need documentation.

Idk. Maybe suggesting using a custom prefix if you create your own tokens? Or outlining all the token types we have so that people can avoid conflicts.

But as I mentioned above, I am not necessarily a "fan" of adding a prefix (mainly because of the significant resource demand). And I also don't really have a point for 3. yet. Just wondering if there is any.

@lukasoppermann
Copy link
Contributor Author

@mperrotti @langermank I updated the ADR to reflect this decision. Do you feel this makes sense now?

@github-actions github-actions bot temporarily deployed to Preview (Storybook) December 9, 2024 19:42 Inactive
@github-actions github-actions bot temporarily deployed to Preview (Storybook) December 9, 2024 19:45 Inactive
@lukasoppermann lukasoppermann added the skip changeset Apply to PRs that should not result in a version bump. label Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset Apply to PRs that should not result in a version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants