Open opi opened 7 years ago
Thanks @opi! Feel free to submit PRs for any/all of these changes if you like :)
@jenlampton there is a PR already ;)
Yes, I see it there now. Not sure how I missed that last time... Thank you @opi! (I will now add the relevant tags...)
Amazing @opi! Love it! We might consider adding StyleLint
to our build process to validate code style.
One issue I saw repeated in the CSS (not a very big deal) is that it adds empty lines before comments, even if that is the first rule in the selector. e.g.
fieldset {
+
/* Webkit/Blink fix for mobile, see stackoverflow.com/questions/17408815 */
min-width: 0;
margin-bottom: 1em;
padding: 0.5em;
}
That new line is unnecessary. It makes sense in some other cases (but not all). Could we remove that rule and reroll the PR?
However this PR does SO MUCH more good than harm, if it's tedious to reroll I'd like to merge it anyway.
Actually the new-lines added throughout Basis are quite excessive. We need these to be fixed before merging.
Actually the new-lines added throughout Basis are quite excessive. We need these to be fixed before merging.
Do you mean that you agree with https://github.com/backdrop/backdrop/pull/1946/files#diff-ac5dbf064787a2cf66646b05c3f17c19R9, but not https://github.com/backdrop/backdrop/pull/1946/files#diff-ac5dbf064787a2cf66646b05c3f17c19R181 ?
I'm not sure that I can remove these extra new line automatically, and probably need to do it manually :/ Also, I think adding new lines always improve code readability, IMHO
I'm planning to make a few PRs related to this issue:
Here's a PR that adds the Stylelint rules: https://github.com/backdrop/backdrop/pull/3516
It uses the 'stylelint-config-recommended' rules by default, but then disables some of them. It then adds a bunch of others based mostly on our CSS standards.
Here's the PR to fix existing issues in core CSS: https://github.com/backdrop/backdrop/pull/3517
Based on the stylelint rules in the above PR.
I think we need to lock in the set of rules we check against first, then we can fix core, then we can setup a GitHub action to fix future PRs. But in order to know which rules works for us, we need to see them in action. Hence the two PRs above.
Nice! :)
How would the action work? Would it mark issues as NW for all PRs that don't pass?
(I was unable to make the issue / PR relationship work last te I tried this. Curious to see how you get that going)
On Mon, Jan 25, 2021, 3:18 PM Peter Anderson notifications@github.com wrote:
Once we can lock in the set of rules we check against, I'll then work on implementing a GitHub action to automate this in future.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/2785#issuecomment-767174856, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBER767ZUNC2UKSTZTGFTS3X33NANCNFSM4DWCRBJA .
@jenlampton I haven't tried the GHA part yet, but I imagine it'll work similar to how the current tests work - just displays an error for the CSS standards test in the PR, then it's up to the author/reviewer to mark the related issue as 'needs work' manually.
But I can certainly see if I can get all that to happen automatically instead, if that's preferred...
Failing a test would be enough :) The problem I ran into was that the PRs and Issues were in different projects and none of the APIs seemed to support that.
Thanks so much for working on automating this as much as possible @BWPanda 🙏 ...and wow! ...I hadn't realized that our code had so many (obvious) standard violations/inconsistencies.
Applauds and thank you all!
A word of warning: changing color definitions from long to short, for instance background: #dddddd;
to background: #ddd;
will have negative side effects. The color module will not be able to replace these values, as it relies on the long form. Which means, that color presets might break.
@indigoxela Could you please add your feedback, and maybe an explanation, here then? https://github.com/backdrop-ops/api.backdropcms.org/issues/50
If we can get that issue actioned first, I can change this one to suit.
I've made a PR to fix Color module and make it properly support short hex codes: https://github.com/backdrop/backdrop-issues/issues/4913
If that gets support and is merged, then theme authors can adhere to our coding standards and we don't need to change them 😃
I have updated the Stylelint rules PR to change CSS colors from short to long as per the recent decision to change the CSS coding standards in that regard. However I can't recall how I ran the stylelint rules against core in order to update the second PR, so that'll have to wait until I have time to look into this again...
So looking into this again after 12 months, it seems that Stylelint has since decided to stop supporting 'stylistic' rules and just focus on rules that help fix bad code, find bugs, etc. The argument, as stated by Prettier is "use Prettier for formatting and linters for catching bugs!" This sounds good and I agree with their premise.
What that means for us then is that we need to use two tools - one (Stylelint) for checking that our CSS conforms to the rules, standards, etc. (doesn't have bugs), and another (~I recommend Prettier~ EDIT: not so sure about this anymore, it doesn't seem very customisable, still looking into it...) for checking the style of the code (whitespace in the right places, lowercase hex colours, etc. - things that aren't bug but that help readability, consistency, etc.).
So on that note, I suggest we re-purpose this issue to focus just on Stylelint and its non-stylistic ruleset, and I'll open a new issue to focus on using Prettier for styling our code nicely.
I've updated the PR that adds Stylelint rules to core (removing the deprecated stylistic ones): https://github.com/backdrop/backdrop/pull/3516 And the PR that applies these CSS standards to core and fixes a bunch of stuff: https://github.com/backdrop/backdrop/pull/4026
BackdropCMS provides some guidelines about CSS Coding Standards but a lot of core files does not respect them.
Tools
StyleLint with custom configuration file and ignore list (I don't want to patch third party stylesheets). Linting command is:
stylelint "core/**/*.css" --ignore-path stylelint-ignore.txt --config stylelintrc.json
.find core/ -type f -not -path core/misc/ckeditor -not -path core/misc/ui -not -path core/misc/smartmenus -not -path core/module/simpletest -name '*.css' -exec sed -i -E 's/:(\S+);/: \1;/g' {} \;
to add a whitespace betweenproperty
andvalue
.find core/ -type f -not -path core/misc/ckeditor -not -path core/misc/ui -not -path core/misc/smartmenus -not -path core/module/simpletest -name '*.css' -exec sed -i -E 's@#([A-F]{3,6});@#\L\1;@g' {} \;
to lowercase hex values.find core/ -type f -not -path core/misc/ckeditor -not -path core/misc/ui -not -path core/misc/smartmenus -not -path core/module/simpletest -name '*.css' -exec sed -i -E 's@#([0-9a-fA-F])\1([0-9a-fA-F])\2([0-9a-fA-F])\3;@#\1\2\3;@g' {} \;
to use shorthand hex values.Related: