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

[eslint plugin] Update polaris-no-bare-stack-item and polaris-prefer-sectioned-prop #371

Merged
merged 5 commits into from
Mar 9, 2023

Conversation

laurkim
Copy link
Contributor

@laurkim laurkim commented Mar 9, 2023

Description

Part of Polaris#8185.

The Polaris style guide was updated to group components into categories. This updates links to Polaris components to reflect those changes.

Additionally, Stack and Card have been deprecated in favor of LegacyStack and LegacyCard.
I've updated the polaris-no-bare-stack-item and polaris-prefer-sectioned-prop rules and tests to use the legacy components instead.

@laurkim laurkim requested a review from kyledurand March 9, 2023 15:43
@laurkim laurkim self-assigned this Mar 9, 2023
@laurkim laurkim marked this pull request as ready for review March 9, 2023 15:45
Copy link
Contributor

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

@laurkim laurkim merged commit 8f5cc11 into main Mar 9, 2023
@laurkim laurkim deleted the lo/update-polaris-component-urls branch March 9, 2023 15:50
@BPScott
Copy link
Member

BPScott commented Mar 24, 2023

@laurkim / @kyledurand: Why is this this marked as a patch release level change? From what I can tell this changes the rules so that they no longer apply to Card and Stack components - that is removing behaviour which is a breaking change. As a consumer of this package I would be very surprised to find out that my linting of Card/Stack stopped working just because I did a yarn update and pulled in a new patch version which by convention is for bugfixes only.

Even if this were to be changed so that it acted upon all of Card/Stack/LegacyCard/LegacyStack - retaining existing behaviour instead of removing it I'd say that's a new feature and it should be a minor change.

What would you like to do here? I see two avenues, and I'd suggest the first:

  • Modify this so that Card/Stack/LegacyCard/LegacyStack are considered in the lint rules and change this so that it is a minor change.
  • Leave the code as is and change the changeset type to be major. At that point I would expect a bunch more information in the changeset in order to inform users that the new major version is for Polaris v11 consumers.

@laurkim
Copy link
Contributor Author

laurkim commented Mar 24, 2023

@BPScott This was originally opened with the intent just to change the URLs (in a patch change) and then I saw the rules and thought it would be ideal to update them. It was an error on my part to leave this as a patch. I can revert this and re-add the changes in 2 PRs, a patch change to update the links and a minor change to update the rules so that Card and Stack as well as their legacy components are considered.

@BPScott
Copy link
Member

BPScott commented Mar 24, 2023

No need to revert and redo from scratch over 2 PRs, I'm fine with just one new PR to fix forwards. It's trivial to update the existing changeset file with the new target of minor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants