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: modules/help/help.css (and help-rtl.css)
Comment | File | Size | Author |
---|---|---|---|
#12 | rtl-before-patch.png | 169.51 KB | marknorris |
#12 | rtl-after-patch.png | 166.75 KB | marknorris |
#11 | rtl-before-patch.png | 165.86 KB | marknorris |
#11 | rtl-after-patch.png | 162.24 KB | marknorris |
#9 | before patch.png | 188.64 KB | marknorris |
Comments
Comment #1
RobLoachhttps://gist.github.com/3006391#L6157
Comment #2
droplet CreditAttribution: droplet commentedmissing RTL
Comment #3
RobLoachThere weren't any errors in help-rtl originally :-) . https://gist.github.com/3006391#L6154
Comment #4
RobLoachActually, yeah, let's change those definitions to margin too. You're right.
Comment #5
barraponto CreditAttribution: barraponto commentedChanging paddings to margins everywhere.
Comment #6
RobLoachComment #7
xjmCSS changes need manual testing in all browsers and all affected core themes:
Post before-and-after screenshots for one browser/theme.
Comment #8
xjmComment #9
marknorris CreditAttribution: marknorris commentedManual testing of patch using post #7 instructions
Screenshots of before and after application of patch are attached.
Site and browser caches cleared after application of patch.
Verified no warnings or errors with css lint tool.
Test passed
Comment #10
xjmCan we also test with an RTL language please?
Comment #11
marknorris CreditAttribution: marknorris commentedI have had another go at testing this using an RTL language.
I am new to this so I will explain what I have done and hopefully it is helpful to this issue...
I have set the language to Hebrew.
I do not have the Hebrew translations (and I am not sure of the correct way to do this for D8) so it is displaying the English text Right to Left. As we are interested here in the CSS then I am assuming that this is not an issue for this test?
When I visit any of the admin pages using the Overlay then I get a white screen on the overlay regardless of whether the patch has been applied or not.
If I use the path in the address bar of /he/admin/help then I get the page as displayed in the image rtl-before-patch.png.
I have applied the patch and cleared site and browser caches.
It is looking okay.
Then I still have the same issue with the overlay but when I go to /he/admin/help then it displays correctly as in rtl-after-patch.png
I have checked it with Inspect Element it to ensure the patch has taken effect.
Comment #12
marknorris CreditAttribution: marknorris commentedHave attempted this test again for RTL using a different machine and version of Safari and the overlay worked okay.
The test has passed.
Comment #13
barraponto CreditAttribution: barraponto commentedI am reading RTBC in the above comment.
Comment #14
webchickCommitted and pushed to 8.x. Thanks!
Comment #15
xjmThanks @marknorris and @barraponto!