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.
Problem/Motivation
See: #1995272: [Meta] Refactor module CSS files inline with our CSS standards
Proposed resolution
Review the CSS against our standards, see:
http://drupal.org/node/1887918#best-practices
https://www.drupal.org/node/2408617
#1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core
Remaining tasks
Review current CSS
Write a patch to fix the suggestions
Run CSSlint against the new CSS
User interface changes
None
API changes
None
Comment | File | Size | Author |
---|---|---|---|
#12 | interdiff.txt | 827 bytes | bjmac |
#12 | file-css-2485431-12.patch | 1.71 KB | bjmac |
#10 | interdiff.txt | 566 bytes | bjmac |
#10 | file-theme-css-2485431-10.patch | 1.28 KB | bjmac |
#8 | file-theme-css-2485431-7.patch | 565 bytes | bjmac |
Comments
Comment #1
rudraram CreditAttribution: rudraram at Axelerant commentedCss lint is clean in file.admin.csseven though div.ajax-progress-bar is being used.
Css lint is clean in file.theme.css except for usage of same background-image twice.
Comment #2
saki007sterMerged both the selectors, removing csslint issues.
Comment #3
saki007sterComment #4
LewisNymanNice! Not much left to fix:
It seems there is blank line at the start of this file.
We can convert this to be a single line comment, like:
/* File icons */
There is a blank line here we can remove.
Comment #5
LewisNymanAh I forgot about file.admin.css!
This has a blank line at the top we can remove
This can be a single line comment
Can we remove the div from this selector?
Comment #6
bjmac CreditAttribution: bjmac commentedWorking on this as part of drupaldevdays sprint
Comment #7
jannis CreditAttribution: jannis commentedi will not start working on this since i saw someone else had laid claim to it
Comment #8
bjmac CreditAttribution: bjmac at Mediacurrent commented- Removed space at the top of the file
- Made comment a single line
- removed div - Tested by uploading new file
Comment #9
rbayliss CreditAttribution: rbayliss commentedNeeds to be merged with the patch from comment 2 above.
Comment #10
bjmac CreditAttribution: bjmac at Mediacurrent commentedCombining with patch from comment 2 and adding an interdiff
Comment #11
oraculi CreditAttribution: oraculi as a volunteer commentedTested patch; looks good.
Comment #12
bjmac CreditAttribution: bjmac at Mediacurrent commentedUpdates to file-theme.css as noted in comment 4
Comment #13
LewisNymanThanks, this looks good.
Comment #14
xjmThis issue only changes CSS, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase.
So this is the only change that seems like it could hypothetically break anything. Since we don't have any manual testing documented on the issue, I checked usage of this class just to get an idea if anything would be impacted:
Committed and pushed to 8.0.x. Thanks everyone!