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.

  1. Copy and paste the the stylesheet(s) below into the css lint tool at http://csslint.net and test.
  2. Fix any warnings or errors the tool finds.
  3. Patch Drupal 8 locally and make sure the css changes have not broken anything visually.
  4. Create patch and upload for the testbot.

Files: modules/help/help.css (and help-rtl.css)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Status: Active » Needs review
FileSize
387 bytes

https://gist.github.com/3006391#L6157

rob@Computron /var/www/drupal/8 $ node csslint/release/npm/cli.js core/modules/help

csslint: No errors in /var/www/drupal/8/core/modules/help/help-rtl.css.

csslint: No errors in /var/www/drupal/8/core/modules/help/help.css.

droplet’s picture

Status: Needs review » Needs work

missing RTL

RobLoach’s picture

Status: Needs work » Needs review

There weren't any errors in help-rtl originally :-) . https://gist.github.com/3006391#L6154

RobLoach’s picture

Status: Needs review » Needs work

Actually, yeah, let's change those definitions to margin too. You're right.

barraponto’s picture

Status: Needs work » Needs review
FileSize
808 bytes

Changing paddings to margins everywhere.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

CSS changes need manual testing in all browsers and all affected core themes:

  1. Test pages where the relevant classes are used without the patch applied.
  2. Apply the patch, clear the site and browser caches, and test again. Confirm that there are no changes or regressions.

Post before-and-after screenshots for one browser/theme.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
marknorris’s picture

FileSize
190.67 KB
188.64 KB

Manual 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

xjm’s picture

Can we also test with an RTL language please?

marknorris’s picture

FileSize
162.24 KB
165.86 KB

I 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.

marknorris’s picture

FileSize
166.75 KB
169.51 KB

Have attempted this test again for RTL using a different machine and version of Safari and the overlay worked okay.
The test has passed.

barraponto’s picture

Status: Needs work » Reviewed & tested by the community

I am reading RTBC in the above comment.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

xjm’s picture

Thanks @marknorris and @barraponto!

Automatically closed -- issue fixed for 2 weeks with no activity.