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

docs(versioning): update versioning guide with css custom property changes #4612

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

joshblack
Copy link
Member

Saw this scenario come up recently and wanted to make sure it was documented with respect to corresponding semver change 👀

Changelog

New

Changed

  • Add section to versioning guide around changing CSS Custom Properties for a component

Removed

Rollout strategy

  • None; if selected, include a brief description as to why

This is a change to project documentation

Testing & Reviewing

I would love to get your feedback on this entry in the versioning doc. Here are some questions if none come to mind to get things going 👀

  • What do you think of this decision? Is it too broad? Too specific?
  • Are there any cases to changing a CSS Custom Property that this entry misses?
  • Do you agree or disagree with the decision for the semver bump annotation for this type of change?

@joshblack joshblack requested a review from a team as a code owner May 20, 2024 17:59
Copy link

changeset-bot bot commented May 20, 2024

⚠️ No Changeset found

Latest commit: 89ad418

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

@joshblack
Copy link
Member Author

Wanted to get your thoughts on this @langermank @mperrotti if you have the time! Let me know if I can provide any additional context for this.

Also wanted to include you @lukasoppermann as an FYI in case this was of interest due to overlap with primitives 👀 The work you did with Banner prompted this and I wanted to make sure this was captured in our versioning guide 👍

Copy link
Contributor

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 88.53 KB (0%)
packages/react/dist/browser.umd.js 88.84 KB (0%)

Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

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

What do you think of this decision? Is it too broad? Too specific?

I'm ok with it, but you could also use this logic to make the argument that changing most other CSS properties could also be considered a breaking change. For example: if we add or remove a position: relative from a component's styles, we change the layout logic and stacking context that consumers might have been relying on in their external styles.


Are there any cases to changing a CSS Custom Property that this entry misses?

Not that I can think of.


Do you agree or disagree with the decision for the semver bump annotation for this type of change?

Agree, but I can easily see people forgetting to do the bump.

occurs when a consumer of the component is setting the value of this CSS Custom
Property in some way expecting a change in the component. For example:

```diff
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah, I didn't know there was a diff flag for code blocks. Nice!

contributor-docs/versioning.md Show resolved Hide resolved
```

```diff
// This is a breaking change if base-size-6 is defined in the next major
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a breaking change even though --space-small and --base-size-6 come from Primer Primtives and we provided a fallback?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a great point about the fallback. You're totally right that the fallback would still apply so there would not be any breakage in the UI, in this case what I'm trying to think through is if this would be a breaking change when someone anticipated --space-small to be the token applied to a component and then it changes to a different token.

Put another way:

  • ExampleComponent depends on space-small
  • Consumer uses/leverage/extends space-small
  • ExampleComponent no longer depends on space-small, uses base-size-6

I'm down either way on this, totally happy to say that the relationship between Component -> token can change and should not be a part of the contract here.

Copy link
Contributor

@mperrotti mperrotti Jun 6, 2024

Choose a reason for hiding this comment

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

I think that the relationship between Component and token can change and should not be part of the contract here.

@joshblack joshblack added this pull request to the merge queue Jun 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2024
@joshblack joshblack added this pull request to the merge queue Jun 21, 2024
Merged via the queue into main with commit 48b43d8 Jun 21, 2024
35 of 36 checks passed
@joshblack joshblack deleted the docs/update-versioning-guide-css-custom-properties branch June 21, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants