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

CommentFileSizeAuthor
#12 interdiff.txt827 bytesbjmac
#12 file-css-2485431-12.patch1.71 KBbjmac
#10 interdiff.txt566 bytesbjmac
#10 file-theme-css-2485431-10.patch1.28 KBbjmac
#8 file-theme-css-2485431-7.patch565 bytesbjmac
#2 file-theme-css-2485431-2.patch741 bytessaki007ster
file.admin_.css_.txt315 byteslewisnyman
file.theme_.css_.txt1.54 KBlewisnyman

Comments

rudraram’s picture

rudraram commented

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

saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
StatusFileSize
new file-theme-css-2485431-2.patch741 bytes

Merged both the selectors, removing csslint issues.

saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
Status: Active » Needs review
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Needs work
Issue tags: +Novice

Nice! Not much left to fix:


/**
 * @file
 * Default style for file module.
 */

It seems there is blank line at the start of this file.

/**
 * File icons.
 */

We can convert this to be a single line comment, like: /* File icons */

  background-position: left center; /* LTR */
}

[dir="rtl"] .file {

There is a blank line here we can remove.

lewisnyman’s picture

lewisnyman
he/him
commented

Ah I forgot about file.admin.css!

  1.  
/**
 * @file
 * Admin stylesheet for file module.

    This has a blank line at the top we can remove 

  2. /**
 * File upload widget.
 */

    This can be a single line comment 

  3. .form-managed-file div.ajax-progress-bar {

    Can we remove the div from this selector?

bjmac’s picture

bjmac commented

Working on this as part of drupaldevdays sprint

jannis’s picture

jannis commented

i will not start working on this since i saw someone else had laid claim to it

bjmac’s picture

bjmac commented
Status: Needs work » Needs review
StatusFileSize
new file-theme-css-2485431-7.patch565 bytes

- Removed space at the top of the file
- Made comment a single line
- removed div - Tested by uploading new file

rbayliss’s picture

rbayliss commented

Needs to be merged with the patch from comment 2 above.

bjmac’s picture

bjmac commented
StatusFileSize
new file-theme-css-2485431-10.patch1.28 KB
new interdiff.txt566 bytes

Combining with patch from comment 2 and adding an interdiff

oraculi’s picture

oraculi commented

Tested patch; looks good.

bjmac’s picture

bjmac commented
StatusFileSize
new file-css-2485431-12.patch1.71 KB
new interdiff.txt827 bytes

Updates to file-theme.css as noted in comment 4

  • Removed extra space at the top and bottom of the file
  • Made comment a single line where possible
  • Removed empty lines between rules
lewisnyman’s picture

lewisnyman
he/him
commented
Status: Needs review » Reviewed & tested by the community

Thanks, this looks good.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

+++ b/core/modules/file/css/file.admin.css
@@ -16,6 +13,6 @@
-.form-managed-file div.ajax-progress-bar .bar {
+.form-managed-file .ajax-progress-bar .bar {

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:

grep -r "ajax-progress-bar" *
core/misc/ajax.js:      this.progress.element = $(progressBar.element).addClass('ajax-progress ajax-progress-bar');
core/modules/file/css/file.admin.css:.form-managed-file div.ajax-progress-bar {
core/modules/file/css/file.admin.css:.form-managed-file div.ajax-progress-bar .bar {
core/modules/file/file.js:        $clickedButton.closest('div.form-managed-file').find('div.ajax-progress-bar').slideDown();
core/modules/system/css/system.module.css:.ajax-progress-bar {

Committed and pushed to 8.0.x. Thanks everyone!

  • xjm committed 126a1f3 on 8.0.x 
    Issue #2485431 by bjmac, saki007ster, LewisNyman, rudraram: Clean up...

Status: Fixed » Closed (fixed)

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