Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Sub-issue of #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core
Inline with the CSS cleanup efforts of the HTML5 initiative, using CSSLint at http://csslint.net provides a quick way to code-sniff our css and tweak styles.
- Copy and paste the the stylesheet(s) below into the css lint tool at http://csslint.net and test.
- Fix any warnings or errors the tool finds.
- Patch Drupal 8 locally and make sure the css changes have not broken anything visually.
- Create patch and upload for the testbot.
Files: core/modules/user/css/user.module.css and user.admin.css
Beta phase evaluation
Issue category | Task because it does not add any features or fixes any bugs |
---|---|
Issue priority | Normal because it doesn't affect any major systems |
Unfrozen changes | Unfrozen because it only changes css |
Prioritized changes | The main goal of this issue is fixing css coding standards |
Disruption | Not disruptive |
Comment | File | Size | Author |
---|---|---|---|
#65 | 1663210-64-user-edit-after.png | 131.73 KB | idebr |
#65 | 1663210-64-permissions-after.png | 564.99 KB | idebr |
#65 | 1663210-64-account-settings-after.png | 189.48 KB | idebr |
#65 | 1663210-64-install-after.png | 595.23 KB | idebr |
#64 | clean-up-user-css-1663210-64.patch | 6.89 KB | mortendk |
Comments
Comment #1
RobLoachhttps://gist.github.com/3006391#L5846
Comment #2
Manuel Garcia CreditAttribution: Manuel Garcia commentedThe #permissions id is for the table, which is ok in my book to keep the namespace there.
Similar reasons go for the rest of ID warnings
Then comes the question of the warning of qualifiying an h2, which is not a valid warning, since you can have several h2's in a single page afaik.
Apparently that's used by profile module which is no longer in core. Which then leads to the question of wether or not we shuould have that blob of css in user module. I've left it untouched since I think it's outside of the scope of this issue.
The attached patch fixes the rest.
Comment #3
Manuel Garcia CreditAttribution: Manuel Garcia commentedComment #4
sdmunoz CreditAttribution: sdmunoz commentedI tested and reviewed the changes and all is good. But would it be better to change #permissions id to a class name generated like?
.permissions-processed .module
thanks
Comment #5
ZenDoodles CreditAttribution: ZenDoodles commentedYes.
...fails csslint
Comment #6
ishmael-sanchez CreditAttribution: ishmael-sanchez commentedPersonally, I agree with @sdmunoz I like defining class names, especially in modules, so that it can be overridden without excessive CSS specificity later. Also by doing so, it reduces CSS lint warnings down to 3 from 8.
Comment #7
atu CreditAttribution: atu commentedWe have a real problem with this declaration, because csslint doesn't allow to write padding, and border with width:
How we can resolve it ?
Comment #8
ishmael-sanchez CreditAttribution: ishmael-sanchez commented@atu I think we can actually just remove that border rule for the following reasons
Comment #9
ishmael-sanchez CreditAttribution: ishmael-sanchez commentedOk I took a shot at this, removed the border (possibly add to Seven instead) and fixed some of the CSS specificity issues. I did leave a couple overqualified selectors in the user-rtl.css so if that's used it would trump the other definitions, but I have never worked on a RTL website so I wasn't 100% if that's how it works.
Comment #10
atu CreditAttribution: atu commented@ishmael-sanchez Did you test your change ? because we have 2 tag with the same class 'password-confirmation', that's why it's precise with a div.
Comment #11
ishmael-sanchez CreditAttribution: ishmael-sanchez commented@atu yes I tested it and it seemed to work. However, it could use more testing.
Comment #12
atu CreditAttribution: atu commented@ishmael Ok cool ;)
Comment #13
rootworkThis needed to be re-rolled.
Comment #14
rootworkTagging from the global sprint.
Comment #15
alansaviolobo CreditAttribution: alansaviolobo commentedrerolled
Comment #17
LewisNyman CreditAttribution: LewisNyman commentedComment #18
nitishchopra CreditAttribution: nitishchopra commentedpatch #13 has worked fine
Comment #19
LewisNyman CreditAttribution: LewisNyman commentedNeeds a reroll but I need to review this patch because there are more changes to make to fix everything.
Comment #20
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedComment #21
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedNeeds review
Comment #22
LewisNyman CreditAttribution: LewisNyman commentedLooks like the latest patch is missing most of the changes in #15
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedthe reroll on #15 seems to be wrong.. that's the
current headclose to the current head(different line numbers just..) just and #20 is probably based on that, so we should really be rerolling #13..? but then again #13 is missing alot of stuff from #9.. is it just me or are you confused as well?Comment #24
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedComment #25
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedComment #26
prajaankit CreditAttribution: prajaankit commented#21
Comment #27
Vikas.Kumar CreditAttribution: Vikas.Kumar commentedComment #28
rootworkThis looks good. I had a minor question about whether the .even and .odd classes were still being used (because we had removed them from D8 in favor of CSS3 pseudo-classes) but I went back and looked at the change record and we are still using those classes on table rows.
I'll circle back and do some screenshots unless someone else gets to it first today.
Comment #31
rootworkWhoops -- turns out the .odd and .even classes were removed, because the patch no longer applied. Here's a reroll without them.
Comment #34
LewisNyman CreditAttribution: LewisNyman commentedAbove here can we add a comment that this is for the permission page?
I think we can remove this styling. It seems weird to have this styling only for this page. This also changes the font size to be below 10px which is way too small
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commented1. Added comment
2. Removed the whole styling like you asked.
Comment #36
mortendk CreditAttribution: mortendk commentedlooks good
Comment #37
idebr CreditAttribution: idebr commentedUpdated the title to reflect the contents of the patch and included a beta evaluation.
Comment #38
idebr CreditAttribution: idebr commentedComment #39
LewisNyman CreditAttribution: LewisNyman commentedCsslint shows no errors for user.admin.css but there are still some errors in user.module.css
Comment #40
rootworkI'm not sure why csslint doesn't complain about the other element names (input.password-confirm, div.password-confirm), but it doesn't, so here's an updated patch with the errors corrected.
Comment #41
Manjit.Singh@lewis: I have remove errors that you have mention in screenshot. Please review patch.
Comment #42
rootwork@Manjit.Singh I think maybe you just posted your interdiff? That patch doesn't have the original changes in it.
I'm re-posting my patches from #40 to run them through the tests.
Comment #43
Manjit.Singh@rootwork: Actually i have updated issue queue at the same time when you were updated.
Thanks for update. :)
Comment #44
crazyrohila CreditAttribution: crazyrohila commented@rootwork: Still there is an overqualified selector.
div.password-suggestions
Comment #45
rootwork@crazyrohila if you look at my comment in #40 you'll see I mentioned that (there are actually two). Csslint doesn't seem bothered by them, so I was waiting to see what Lewis thought.
@Manjit.Singh yes I saw that our posts crossed each other. But apart from that, the patch you uploaded only included the changes _you_ made, not the changes from the earlier patches. (Your own changes are usually included in an interdiff.)
Comment #46
Manjit.Singh@rootwork Actually i was thinking, lets remove overqualified selector from
input.password-confirm, div.password-confirm.
Is it make sense.Comment #47
mortendk CreditAttribution: mortendk commentedwe have a class with the .password-confirm classes that are used both on an input & the div.
This patch:
* renames the information
.password-confirm
to.password-confirm-match
in the user.js file, else we cant remove the input & div.* seven had to get that fix in as well, why that css file is also patch
* adds a @file in the top of the user.module.css
attached the followup issue, to make the file naming correct (following the MAT) that should be worked on after the csslint is checked, to not create to much creep.
Screenshots added as well for the 2 places that are effected for the classrenaming
change of classname:
seven install
Comment #48
mortendk CreditAttribution: mortendk commentedattached missing screenshot from seven install with the new
.password-confirm-match
classComment #49
joelpittetThere is no .permissions class on the page let alone the same table the ID is on.
Comment #50
mortendk CreditAttribution: mortendk commented@joel good catch have fixed it to select on the form instead of the table id
I have added screenshots to show that theres no regression
Patched:
Not patched:
Comment #51
mortendk CreditAttribution: mortendk commentedheres the correct patch :/
change the selector on the permission page to
.user-admin-permissions .module
Comment #52
mortendk CreditAttribution: mortendk commented*removed a double bracket
* added in
permission class
for the system page, so we the selector there.screenshot for the permission page:
Comment #53
joelpittetThanks @mortendk this is looking better:
double dot.
Likely need a screenshot for this.
Should the section header exist too for the password strength indicator? I'm curious because I don't know.
Does this need div? That should be caught by linter, I'd assume.
Comment #54
mortendk CreditAttribution: mortendk commented1. ups ..
2. user-admin-settings dont exits anywhere, so i guess its a leftover from way back.
3. yes it should have cause its a @file afaik
4. yes cleaned up in this patch
Comment #55
joelpittet/admin/config/people/accounts
Just thinking they are both needed? @file block on top and the password strength below it?
Comment #56
mortendk CreditAttribution: mortendk commentedOk added it back in & fixed the selector to be a class instead of the ID
with patch
without patch
Comment #57
joelpittetNot what I meant but close:P Was thinking separate comment blocks. Sorry should've been explicit.
I'll leave this as NR, for a second pair of eyes but other than that I think it's RTBC.
Comment #58
mortendk CreditAttribution: mortendk commentedim gonna scope creep this issue a bit and fix the MAT file naming [#1887922] as well now where at it, instead of having it as an seperate issue (as noted earlier)
This patch now separate the visual styling of the password strenght generator.
No module.theme.css
with module.theme.css
Default gray tones are set for the strength indicator, to remove any "colors" (yes gray is a color, but now its clear for the themer that theres "no styling")
Comment #59
idebr CreditAttribution: idebr commentedUpdated the tags with 'Seven', since it also touches seven css.
Comment #60
joelpittet@mortendk aww, and I was about to RTBC it... scope--
Can you please put up an interdiff on that so I don't have to review from scratch?
Comment #61
mortendk CreditAttribution: mortendk commentedi know but it seemed silly not to get it all done with the filenameing - i totally scope creeped my self.
heres the interdiff
Comment #62
mortendk CreditAttribution: mortendk commentedComment #63
idebr CreditAttribution: idebr commentedThis styling has been copied over from user.icons.css, but the original file is still there. In #2421373: rename *.icons.css files to *.icons.theme.css the proposed resolution is to rename user.icons.css to user.icons.theme.css, so the styling here should not be copied over.
Comment #64
mortendk CreditAttribution: mortendk commentedremoved the left over icons
Comment #65
idebr CreditAttribution: idebr commentedAll User module CSS files now pass CSSLint, the css selectors have been split into admin/module/theme.css and fixes a CSS selector that previously no longer applied (
#permissions td.module)
.Screenshots annotated with relevant classes:
Comment #66
idebr CreditAttribution: idebr commentedComment #67
webchickGreat work on the thorough testing here.
Committed and pushed to 8.0.x. Thanks!