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

fix: reduce excess stylelint warnings from bundle module #3655

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented Apr 3, 2025

Description

Fixes large number of stylelint warnings appearing after yarn start. These were the result of stylelint running on the
tools/bundle/dist/index.module.css file.

  • Adds stylelint override to better lint module files that have a different class format.
  • Add ignoreFiles for the tools/bundle/dist, as stylelint should not be running on the dist folders during this step; it seems to be disregarding the .stylelintignore file.

CSS-1155

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Pull down the branch locally and:

  • yarn install and yarn start. After Storybook starts up (after loader and pages are functional), you should not see the 9k stylelint warnings related to "._" classes expecting to match pattern. No [stylelint] labeled warnings should appear for the bundle dist file.
  • To test the rule override, delete the ignoreFiles property in the root stylelint.config.js and run yarn start again. There should only be a few dozen stylelint warnings for the module file, instead of 9k.

Screenshots

Original problem:

Screenshot 2025-03-13 at 4 56 52 PM

To-do list

Copy link
Contributor

github-actions bot commented Apr 3, 2025

File metrics

Summary

Total size: 2.24 MB*

🎉 No changes detected in any packages

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Apr 3, 2025

🚀 Deployed on https://pr-3655--spectrum-css.netlify.app

Fixes large number of stylelint warnings appearing after yarn start.
These were the result of stylelint running on the
tools/bundle/dist/index.module.css file.

- Adds stylelint override to better lint module files that have a
  different class format.
- Add ignoreFiles for the tools/bundle/dist, as stylelint should not
  be running on the dist folders during this step; it seems to be
  disregarding the .stylelintignore file.
@jawinn jawinn force-pushed the jawinn/fix-excess-stylelint-warnings branch from 12afb4d to 1ac3153 Compare April 3, 2025 14:29
Copy link

changeset-bot bot commented Apr 3, 2025

⚠️ No Changeset found

Latest commit: 1ac3153

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

Copy link
Collaborator

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

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

One note about where we list our ignored files, otherwise this works for me when I pull it down locally.

Comment on lines +17 to +19
ignoreFiles: [
"tools/bundle/dist/*.css",
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm reading this correctly, I think we should be using either the .stylelintignore file or the ignoreFiles property in the config. I'm flexible with whichever approach we use but I'd like to see all the same data in it. Would you mind either migrating the content from the stylelintignore to here or moving this out to the stylelintignore file?

https://stylelint.io/user-guide/ignore-code#files-entirely

Copy link
Collaborator Author

@jawinn jawinn Apr 4, 2025

Choose a reason for hiding this comment

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

Part of the problem is that there appears to be that the .stylelintignore file is disregarded by whatever stylelint is running after yarn start. It seems to be only when stylelint is running from the storybook directory. The existing ignored dist in the .stylelintignore should already cover these files, and it seems to work if I manually run the stylelint command from the root. But not if I run it from .storybook. Should I track that as a separate bug? I wasn't able to track down why; I tried all kinds of variations and path.resolve for the ignorePath setting in the two separate postcss.config.js files, without any luck. So this is somewhat of a workaround for the time being.

Edit: I also could leave this specific change off if we want to address that bug separately. I was on the fence about it.

@@ -235,5 +238,17 @@ module.exports = {
],
}
},
{
/* Module CSS file classes have an underscore before classes, and use underscores instead of a dash as separators (e.g. _spectrum_well) */
files: ["tools/bundle/**/*.module.css"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know if this works if we simplify it to just files:["*.module.css"], - I'm just thinking all module files would have these same linting issues so if we add more, it'd be lovely to be able to inherit these automatically

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That should work. I can test and make that change.

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.

2 participants