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/field_ui/field_ui.css (and field_ui-rtl.css)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

_Cedric_’s picture

Assigned: Unassigned » _Cedric_
Status: Active » Needs review
FileSize
2.8 KB

Needs review

RobLoach’s picture

FileSize
4.11 KB

Missed a couple.

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


csslint: No errors in /var/www/drupal/8/core/modules/field_ui/field_ui.admin-rtl.css.
_Cedric_’s picture

FileSize
4.12 KB

Last css fixes, needs review

droplet’s picture

Status: Needs review » Needs work
+++ b/core/modules/field_ui/field_ui.admin.cssundefined
@@ -4,68 +4,68 @@
+.field-ui-display-overview-form .field-formatter-summary {
...
-#field-display-overview td.field-formatter-summary-cell span.warning {
+.field-ui-display-overview-form .field-formatter-summary-cell .warning {

<table id="field-display-overview" class="field-ui-overview

any reason not to use this class ?

Albert Volkman’s picture

Status: Needs work » Needs review
FileSize
4.01 KB

Not that I can see.

swentel’s picture

Status: Needs review » Needs work
FileSize
21.01 KB

The refresh button at the bottom of the manage display is still there, that's because the button is not inside the table, so the class doesn't work for that.

See screenshot

Screen Shot 2012-10-11 at 23.24.21.png

drupee’s picture

Hey Guys,

Is this issue already fixed?

I have cloned a recent version of git repo. I neither see any warnings nor any 'Refresh' button.
Can you please elaborate this issue if it is still there?

barraponto’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.36 KB

Looks good, issue related by @swentel in #6 is gone as of #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted. Rerolling to apply cleanly.

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

Assigned: _Cedric_ » Unassigned
Status: Reviewed & tested by the community » Needs work
markgifford’s picture

markgifford’s picture

FileSize
57.89 KB
57.89 KB

Before patch applied:

Before patch applied

After patch applied:

After patch applied

No visible difference, all good. Tested in Chrome on Ubuntu btw

markgifford’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs screenshots, -Needs manual testing

Forgot to remove tags and change status after posting screenshots

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

swentel’s picture

Status: Fixed » Needs work

So we missed the 'Manage display' screen, seriously people, see #6.

You now see the 'Refresh' button that should be hidden.

swentel’s picture

Status: Needs work » Needs review
FileSize
413 bytes

Here's the patch.

swentel’s picture

FileSize
1021 bytes

Ok, better approach by using element-invisible.

wuinfo - Bill Wu’s picture

+++ b/core/modules/field_ui/field_ui.admin.cssundefined
@@ -66,6 +66,6 @@
+#edit-refresh {
   display: none;

Rule of css lint: Don't use IDs in selectors.

yched’s picture

Status: Needs review » Reviewed & tested by the community

#17 works.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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