Stylelint install and config#1427
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
demiankatz
left a comment
There was a problem hiding this comment.
As with #1426: do we need to integrate this into package.json scripts and/or GitHub actions, or is it already going to happen "magically" somehow?
|
All conflicts should have been resolved. The stylelint rules file has quite a few rules disabled because they would take quite a bit of work to fix, and should be manually checked.
I've added an npm command to run stylelint, however I haven't added it to the git workflow file because: a) It makes a lot of changes Once this is merged with dev I'll create a new branch and run an auto-fix, then check the changes to make sure nothing breaks. If that's all fine then I think we can add stylelint to the workflow. Then as a longer term job, we can look at turning the disabled rules on one-by-one and fixing things where appropriate. |
There was a problem hiding this comment.
Thanks, @LlGC-jop! A few thoughts:
1.) Maybe the best way to structure the package.json scripts would be to rename the current lint to lint-code and then redefine it as npm run lint-code lint-styles. Then we have a universal lint command, plus the ability to lint things separately.
2.) I realize that following my proposal above would cause changes to get applied, which you might not be ready to move forward with. But my argument is that we shouldn't commit tool configurations that will change code without also committing the code changes. I think it makes sense to do it all together, since if the changes cause problems, we'll have to change the configuration -- so we might as well start with a working configuration, instead of an unproven one. I don't mind if that takes a little extra time to get this PR done.
3.) Semi-related note: I think you wanted to adjust the prettier configuration to look at less files; would it make sense to do that in a separate PR and commit any related changes first? Is it possible that doing that will reduce the number of changes introduced by stylelint?
We can discuss further at today's stand-up!
|
@demiankatz I've had a look over the CSS changes and am largely happy that they're fine - though I won't claim that I haven't missed anything :) I've requested a review from @LanieOkorodudu, if that's ok with you Lanie, just to have another pair of eyes look over this build in case I've missed anything. The few instances I mentioned yesterday where properties were repeated due to -moz and -webkit prefixes being removed were all in the mixins file, most of which aren't being used. So I'm happy to leave these duplicates as they shouldn't affect anything - anyone writing new CSS shouldn't be using prefixes any more afaik and therefore won't need the mixins. |
@LlGC-jop I am afraid, I will not be able to really check this today as I have the whole day course to attend. Will be it okay to request @jamesmisson, if he can spare some time to check. Otherwise I will do it tomorrow. Thanks. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LlGC-jop -- I gave these files a fairly careful review and spotted a handful of things that might be problems (or might just point to my ignorance about details of LESS). :-)
|
@LlGC-jop, I had a look through the changes and also read Demian’s suggestions, based on his detailed feedback and familiarity with the context, I feel he’s probably in the best position to give the most useful review on this one. Also, I just wanted to mention that my expertise in this area isn’t as strong as Demian, so I didn’t feel confident enough to add a formal review or approval. I really appreciate being included, though, and I hope it’s okay that I defer to the expert here. Thanks again! |
|
No problem, @LanieOkorodudu -- let's wait and see what @LlGC-jop decides to do. Once we reach a final revision, if you have time, it might be helpful to do some side-by-side comparisons of the build here against the current dev build. In theory, there should be no visible side effects of reformatting the LESS code, but it may be worth doing some quick comparisons to be absolutely sure nothing has unexpectedly changed. |
… regenerated to add missing deps
|
Hi both, have added comments/made changes, but in short:
I've also merged the latest dev into my branch and regenerated the package lock file, so this should be up-to-date with the recent merge into dev of 4.2.0 that James did yesterday. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LlGC-jop, this all looks good to me now! I'll leave this open in case anyone else wants to test/review before merging. We can discuss at today's standup.
|
I’ve reviewed all the file changes and compared the additions, fixes, and updates, everything looks good to me. I’ll leave the merging to @demiankatz, as he’s happy with all the changes. Thank you, @LlGC-jop, for the massive piece of work you’ve done here, and thanks to @demiankatz for your valuable feedback. |
Adds
stylelintpackage pluslesssupport, as well as config that will overlook the major issues in the project for now. Everything else should be auto-fixable as a future PR.Also adds VSCode settings for the project so
.lessfiles can have issues highlighted (but won't interfere with saving/committing)