-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update changeset guidelines #4510
base: master
Are you sure you want to change the base?
Conversation
|
@toptal-anvil ping reviewers |
@@ -32,6 +32,8 @@ Please use this format: | |||
``` | |||
--- | |||
'@toptal/[package]': [version bump] | |||
//if the build bump is major, please also add the line below to have the changeset comments visible in the Picasso patch release notes. | |||
'@toptal/picasso': patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this comment will always be visible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct! :)
I've changed it, Thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the https://github.com/toptal/picasso/blob/master/packages/topkit-analytics-charts/package.json is bumped, we do not need to bump @toptal/picasso
as it does not depend on it. Should there be some kind of specification that @toptal/picasso
should be bumped not in 100% if cases (as @toptal/picasso
does not depend on all packages in https://github.com/toptal/picasso)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also mention, that when we add the patch for Picasso, we should also include the heading for giving context in the Picasso release notes
@toptal-anvil ping reviewers |
@@ -32,6 +32,7 @@ Please use this format: | |||
``` | |||
--- | |||
'@toptal/[package]': [version bump] | |||
//if the version bump is major, please also add `'@toptal/picasso': patch` and don't forget to include the heading to have the changeset comments visible in the Picasso patch release notes. This is only required for packages that depend on picasso, usually of the form `@toptal/picasso-show-more`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get this: packages that depend on picasso
— maybe we are talking about packages that Picasso depends on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! I will reformulate accordingly! Thx!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When introducing changes to Picasso, it's important to distinguish between updates that require user action and those that don't.
For example, in the case of a Tailwind migration, since Picasso already includes a peer dependency on picasso-tailwind, users don't need to take any additional steps when new components are migrated. Therefore, such changes can be considered non-breaking, and a patch update would suffice.
However, for breaking changes—such as an API modification (e.g., removing the icon property from the List component)—these changes would require users to adjust their implementations. In these cases, a major version bump for Picasso is necessary, rather than a patch.
@@ -32,6 +32,7 @@ Please use this format: | |||
``` | |||
--- | |||
'@toptal/[package]': [version bump] | |||
//if the version bump is major, please also add `'@toptal/picasso': patch` and don't forget to include the heading to have the changeset comments visible in the Picasso patch release notes. This is only required for packages that Picasso depends on, usually of the form `@toptal/picasso-package-name`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//if the version bump is major, please also add `'@toptal/picasso': patch` and don't forget to include the heading to have the changeset comments visible in the Picasso patch release notes. This is only required for packages that Picasso depends on, usually of the form `@toptal/picasso-package-name`. | |
// if the version bump is major, please also add `'@toptal/picasso': patch` and don't forget to include the heading to have the changeset comments visible in the Picasso patch release notes. This is only required for packages that Picasso depends on, usually of the form `@toptal/picasso-package-name`. |
[FX-NNNN]
Description
Update changeset guidelines to include a reference to
'@toptal/picasso': patch
for major bumps.How to test
Screenshots
PR commands
List of available commands:
@toptal-bot run package:alpha-release
- Release alpha version@toptal-anvil ping reviewers
- Ping FX team for reviewPR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:
to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle
)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?