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

chore: update stylelint from v15 to v16 #6515

Merged

Conversation

davidmenendez
Copy link
Contributor

@davidmenendez davidmenendez commented Nov 27, 2024

Closes #6424

major dependency upgrades for stylelint from v15 to v16 and stylelint-plugin-carbon-tokens from 2.8.0 to 3.2.1

addresses breaking changes in the carbon token plugin:

carbon/layout-token-use has been renamed to carbon/layout-use.
carbon/theme-token-use has been renamed to carbon/theme-use.
carbon/type-token-use has been renamed to carbon/type-use.

@davidmenendez davidmenendez requested a review from a team as a code owner November 27, 2024 20:27
@davidmenendez davidmenendez requested review from makafsal and matthewgallo and removed request for a team November 27, 2024 20:27
Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for ibm-products-web-components ready!

Name Link
🔨 Latest commit 4acbeff
🔍 Latest deploy log https://app.netlify.com/sites/ibm-products-web-components/deploys/674f1c488ce9fe0008cfe45f
😎 Deploy Preview https://deploy-preview-6515--ibm-products-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for carbon-for-ibm-products ready!

Name Link
🔨 Latest commit 4acbeff
🔍 Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/674f1c48ba824e0008561297
😎 Deploy Preview https://deploy-preview-6515--carbon-for-ibm-products.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@elycheea elycheea left a comment

Choose a reason for hiding this comment

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

Much better! Some smaller changes and observations —

  1. Are the AI tokens not captured in stylelint-plugin-carbon-tokens? If so, we should check in with Lee.
  2. Caught a few places that look like redundant disables. If we can remove them, we should.
  3. ^ some of those were using a namespace. I would expect stylelint-plugin-carbon-tokens to work with them, in which case they are redundant again. If not, we should follow up with Lee.

margin: 1rem;
// stylelint-disable-next-line carbon/theme-token-use
// stylelint-disable-next-line carbon/theme-use
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// stylelint-disable-next-line carbon/theme-use

Wouldn’t this one count as token use? 🤔

@@ -24,8 +24,8 @@
@keyframes step-content-entrance {
0% {
opacity: 0;
// stylelint-disable-next-line carbon/layout-token-use
transform: translateY(-0.75rem);
// stylelint-disable-next-line carbon/layout-use
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// stylelint-disable-next-line carbon/layout-use

Since this was updated to a token, we can probably remove this line.

@@ -158,7 +158,7 @@

/* This section to be removed after support for slug dropped - start */
.#{$block-class} th.#{$block-class}__with-slug {
/* stylelint-disable-next-line carbon/theme-token-use */
/* stylelint-disable-next-line carbon/theme-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Are AI tokens not captured in the stylelint-plugin-carbon-tokens package or is this just leftover from maybe an earlier version? 👀

@@ -32,8 +32,8 @@
.#{c4p-settings.$carbon-prefix}--popover--bottom-right.#{variables.$block-class}__row-height-settings-popover
.#{c4p-settings.$carbon-prefix}--popover-caret {
// Used to fix layout issue with IconButton caret inside of Popover component
/* stylelint-disable-next-line carbon/layout-token-use */
left: -4px;
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

@@ -29,8 +29,8 @@
@keyframes form-content-entrance {
0% {
opacity: 0;
// stylelint-disable-next-line carbon/layout-token-use
transform: translateY(-0.75rem);
// stylelint-disable-next-line carbon/layout-use
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// stylelint-disable-next-line carbon/layout-use

@@ -13,8 +13,8 @@
display: inline-block;
max-height: 100%;
box-sizing: border-box;
/* stylelint-disable-next-line carbon/layout-token-use */
padding: 1rem;
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

@@ -43,7 +43,7 @@
}

.#{c4p-settings.$carbon-prefix}--list-box__menu-icon {
/* stylelint-disable-next-line carbon/layout-token-use */
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

@@ -9,8 +9,8 @@

.sb-show-main.sb-main-centered #storybook-root {
// Override default center/center with top/center.
/* stylelint-disable-next-line carbon/layout-token-use */
margin-top: 1rem;
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

@@ -12,11 +12,13 @@

// TODO: add any additional styles used by UserAvatar.stories.js
.theme-section {
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

Assuming these should work when namespaced, but if not, we may need to keep this in place for now.

padding: spacing.$spacing-05;
background: theme.$background;
color: theme.$text-primary;
}

.theme-text {
/* stylelint-disable-next-line carbon/layout-use */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line carbon/layout-use */

Again, assuming the namespaced tokens should still work with the plugin.

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.88%. Comparing base (b2298b5) to head (4acbeff).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6515   +/-   ##
=======================================
  Coverage   79.88%   79.88%           
=======================================
  Files         394      394           
  Lines       12877    12877           
  Branches     4258     4258           
=======================================
  Hits        10287    10287           
  Misses       2590     2590           
Components Coverage Δ
ibm-products ∅ <ø> (∅)
ibm-products-web-components ∅ <ø> (∅)

@davidmenendez
Copy link
Contributor Author

@elycheea @matthewgallo looks like everything is building properly now and should be ready for review 👍

elycheea
elycheea previously approved these changes Dec 2, 2024
matthewgallo
matthewgallo previously approved these changes Dec 2, 2024
Copy link
Member

@matthewgallo matthewgallo left a comment

Choose a reason for hiding this comment

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

Nice! I do think we could use a spacing token for the theme dropdown in the examples (examples/carbon-for-ibm-products/*/src/ThemeSelector/_theme-dropdown.scss), I noticed a lot of repeated disabling in those files. But lets take a look after we get these changes in.

@matthewgallo matthewgallo added this pull request to the merge queue Dec 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Dec 2, 2024
@davidmenendez
Copy link
Contributor Author

totally agree. there's a lot of cleanup that we can do around the scss files, specifically around taking a look at anything that's not using tokens and removing redundant code, which seems to be prevalent in storybook specific styles.

@davidmenendez davidmenendez dismissed stale reviews from matthewgallo and elycheea via 4acbeff December 3, 2024 14:57
@davidmenendez
Copy link
Contributor Author

@elycheea & @matthewgallo when you have a sec to re-review 👀 🙇

@davidmenendez davidmenendez added this pull request to the merge queue Dec 3, 2024
Merged via the queue into carbon-design-system:main with commit 0da2372 Dec 3, 2024
34 checks passed
@davidmenendez davidmenendez deleted the stylelint-upgrade2 branch December 3, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade stylelint to v16
3 participants