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

FLUID-6555: add initial configuration for CSS and Sass linting #20

Merged
merged 7 commits into from
Oct 23, 2020
Merged

FLUID-6555: add initial configuration for CSS and Sass linting #20

merged 7 commits into from
Oct 23, 2020

Conversation

greatislander
Copy link
Contributor

This is an initial attempt at FLUID-6555 which uses the Stylelint configuration from inclusive-design/idrc. The configuration rules should be able to be overridden as documented in #17. Hoping to start the discussion from here, including an evaluation of these rules to determine if they are suitable as a starting point for Fluid Project… projects!

@jobara jobara mentioned this pull request Oct 6, 2020
@the-t-in-rtf
Copy link
Collaborator

Based on our experience with Justin's recent pull, I think a good litmus test of this would be to create a "companion pull" against Infusion that uses the work in progress here for its linting. That would allow us to confirm whether there are linting errors in Infusion itself and to talk about either adjusting rules or fixing errors.

@greatislander
Copy link
Contributor Author

greatislander commented Oct 8, 2020

@the-t-in-rtf Yes, that makes sense. In FLUID-6551 I'd proposed exploring alternatives to Stylus; this PR currently doesn't lint Stylus files (there's an experimental Stylus linting extension for Stylelint but I haven't tested it) so it may not have any impact on Infusion.

@the-t-in-rtf
Copy link
Collaborator

At least until the UI components are moved out of the repo, there are still raw CSS files that should be included in linting. See my comment on FLUID-6551.

It would also be nice to have support for Stylus, in looking at your list on FLUID-6551, I realised how many projects would benefit from having that. How much work are we talking?

@greatislander
Copy link
Contributor Author

@the-t-in-rtf I'm going to try updating this PR with some Stylus code and the stylus extension and see if it works. I'll also open a PR against infusion once I've done that.

@greatislander
Copy link
Contributor Author

@the-t-in-rtf So, I've tested a bit, and it's not going to work to have a shared config for CSS, SCSS and Stylus. We can have a shared CSS/SCSS configuration as the syntaxes are more compatible but we would need a separate configuration to lint Stylus.

@greatislander
Copy link
Contributor Author

@the-t-in-rtf I'm going to explore the use of stylint (not to be confused with stylelint) for independently linting Stylus files. (Props to @BlueSlug for suggesting I look for other linters that could handle Stylus.)

@the-t-in-rtf
Copy link
Collaborator

Glad to hear it. I'll wait to hear from you.

@greatislander
Copy link
Contributor Author

@the-t-in-rtf After further investigating, I don't believe that stylint is a viable option. It hasn't been updated in over a year and has a number of reported issues; most significantly the stylint-stylish reporter seems not to work properly with fluid-grunt-lint-all and hasn't been updated since 2018.

There is an "official" linter: https://github.com/stylus/stlint

However, it doesn't have any existing Grunt integrations and if the ultimate goal is what you've outlined in #21, I'm not sure it makes sense to spend time developing a Grunt integration for stlint. Thoughts?

@the-t-in-rtf
Copy link
Collaborator

Thank you for putting in the time to research this, @greatislander. Adding at least some CSS coverage seems better than continuing with none. I will take a look at the other pull shortly.

.stylelintrc Outdated Show resolved Hide resolved
@the-t-in-rtf
Copy link
Collaborator

the-t-in-rtf commented Oct 20, 2020

I think the associated Infusion pull is looking good, I'll check in with @amb26 and see if we can round out this pair of pulls soon. If the work on the standard config is likely to be available, I think it would make sense to:

  1. Review and merge the central config.
  2. Cut a release of the central config.
  3. Update this pull and the infusion pull to use the central config.
  4. Merge this pull and cut a release of fluid-grunt-lint-all
  5. Update the Infusion pull to point to the release of fluid-grunt-lint-all.
  6. Merge the Infusion pull.

@greatislander
Copy link
Contributor Author

@the-t-in-rtf Steps 1-3 are now complete (see: fluid-project/stylelint-config-fluid).

@the-t-in-rtf
Copy link
Collaborator

Update this pull and the infusion pull to use the central config.

The infusion pull still uses a hard-coded .stylelintrc file. It would be good to confirm whether you have to bring in the config as a dev dependency in Infusion, or whether it will bring it in via fluid-grunt-lint-all. I'm assuming it will have to be a dev dependency, but in any case we should at least have a consistent .stylelintrc.json file in the Infusion pull to confirm there are no additional surprises.

@greatislander
Copy link
Contributor Author

@the-t-in-rtf Thanks for the reminder— I've updated that PR. It's still referencing this branch for fluid-grunt-lint-all.

@the-t-in-rtf the-t-in-rtf merged commit b6a67ca into fluid-project:master Oct 23, 2020
@greatislander greatislander deleted the FLUID-6555 branch October 23, 2020 15:09
jobara added a commit to jobara/infusion that referenced this pull request Feb 3, 2021
* greatislander/FLUID-6555:
  FLUID-6555: remove empty blocks and one extraneous comment
  FLUID-6555: remove extraneous comment from Reorderer.css
  FLUID-6555: remove unnecessary comment break
  FLUID-6555: reposition comment in OverviewPanel.css
  FLUID-6555: update fluid-lint-all to 1.0.4
  FLUID-6555: update fluid-lint-all, cover all CSS and SCSS files
  FLUID-6555: update fluid-lint-all, adjust .fluidlintrc.json
  FLUID-6555: fix SCSS error
  FLUID-6555: apply linting rules to Sass
  FLUID-6555: update to fluid-grunt-lint-all 2.0.0
  FLUID-6555: use central config
  FLUID-6555: lint more files
  FLUID-6555: lint more files, apply bracket spacing rules
  FLUID-6555: address CSS linting issues from fluid-project/fluid-grunt-lint-all#20
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.

2 participants