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)

CommentFileSizeAuthor
#17 1662986-17.patch1021 bytesswentel
#16 1662986-16.patch413 bytesswentel
#12 beforepatch.png57.89 KBmarkgifford
#12 afterpatch.png57.89 KBmarkgifford
#8 drupal-csslint-field_ui-1662986-8.patch3.36 KBbarraponto
#6 Screen Shot 2012-10-11 at 23.24.21.png21.01 KBswentel
#5 fieldui_css_cleanup-1662986-5.patch4.01 KBalbert volkman
#3 FieldUiCss-1662986-3.diff4.12 KB_Cedric_
#2 1662986.patch4.11 KBrobloach
#1 FieldUiCss-1662986-1.diff2.8 KB_Cedric_

Comments

_Cedric_’s picture

_Cedric_ commented
Assigned: Unassigned » _Cedric_
Status: Active » Needs review
StatusFileSize
new FieldUiCss-1662986-1.diff2.8 KB

Needs review

robloach’s picture

robloach
he/him
commented
StatusFileSize
new 1662986.patch4.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

_Cedric_ commented
StatusFileSize
new FieldUiCss-1662986-3.diff4.12 KB

Last css fixes, needs review

droplet’s picture

droplet commented
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

albert volkman commented
Status: Needs work » Needs review
StatusFileSize
new fieldui_css_cleanup-1662986-5.patch4.01 KB

Not that I can see.

swentel’s picture

swentel commented
Status: Needs review » Needs work
StatusFileSize
new Screen Shot 2012-10-11 at 23.24.21.png21.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

drupee commented

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

barraponto
he / him
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new drupal-csslint-field_ui-1662986-8.patch3.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

xjm
she/her
Primary language English
commented
Issue tags: +Needs screenshots, +Needs manual testing

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

xjm
she/her
Primary language English
commented
Assigned: _Cedric_ » Unassigned
Status: Reviewed & tested by the community » Needs work
markgifford’s picture

markgifford commented
markgifford’s picture

markgifford commented
StatusFileSize
new afterpatch.png57.89 KB
new beforepatch.png57.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

markgifford commented
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

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

swentel’s picture

swentel commented
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

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 1662986-16.patch413 bytes

Here's the patch.

swentel’s picture

swentel commented
StatusFileSize
new 1662986-17.patch1021 bytes

Ok, better approach by using element-invisible.

wuinfo - bill wu’s picture

wuinfo - bill wu commented 
+++ 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

yched commented
Status: Needs review » Reviewed & tested by the community

#17 works.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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