Posted by ZenDoodles on June 27, 2012 at 4:34pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | file.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Drupalconsydney, html5, Needs manual testing, Needs reroll, Needs screenshot, Needs steps to reproduce, Novice |
Issue Summary
Sub-issue of #1190252: Meta: 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/file/file.css
Comments
#1
test in http://csslint.net found two warnings attach is the patch i have created and tested locally, nothing was broken.
#2
#3
#4
+++ b/core/modules/file/file.admin.cssundefined@@ -21,6 +21,6 @@
+.form-managed-file div.ajax-progress-bar .bar {
div.ajax-progress-bar
#5
hi droplet add a new patch but question why on csslint it has only two warnings? and what you have said on comment no.4 is not a warning or error on css lint?
#6
The last submitted patch, file-css-clean-up-1662990-5.patch, failed testing.
#7
I scanned with my eyes. and I just tested following code and shows me 2 warnings.
form-managed-file div.ajax-progress-bar div.bar {margin: 0;
}
when there is 2 styles, csslint thinks this is indeed needed
.form-managed-file div.ajax-progress-bar {display: none;
margin-top: 4px;
padding: 0;
width: 28em;
}
.form-managed-file div.ajax-progress-bar .bar {
margin: 0;
}
Hehe, #1190252 haven't talking it deeply before this mission.
(even it leaves, HTML5 Clean up will remove it i think.)
#8
#5: file-css-clean-up-1662990-5.patch queued for re-testing.
#9
Wrong component
#10
Everything OK, applies cleanly.
#11
CSS changes need manual testing in all browsers and all affected core themes:
Post before-and-after screenshots for one browser/theme.
#12
#13
It's unclear to me where these classes are being applied. Could someone with better knowledge of the File module point out exactly what a tester should be looking for in this case?
#14
#5 applies well in Firefox, Chrome & IE8.
Will be needed to check the .ajax-progress-bar.
#15
#16
@joedougherty They're added on the file upload boxes in content add/edit screens
#17
Also tested against latest build of Drupal 8, patch in #5 applies fine. Looks great in all Mac browsers and Lint returns 0 errors :)
#18
Removed 2 overqualified elements in file.admin.css as recommended by csslint
Also worked in Safari for Windows, didn't attach screenshot.
#19
@daniel92
does that mean you made a change to the code and then took the screenshots? (great screenshots!!!!)
#20
The left screenshot for each browser is before I changed the code, and the right screenshot for each browser is after I changed the code to prove that it didn't break anything.
Thankyou!!
#21
OH, I see, one of the files attached was a patch! sorry, I guess I thought there were two jpgs or something.
changing status to needs review so the testbot has a look at your patch.
how is this different than the previous patch in #5? maybe an interdiff would help... but also maybe just #5 does not apply anymore.
#22
The last submitted patch, file-css-clean-up-1662990.patch, failed testing.
#23
#24
Rerolled patch.
#25
Tested and it gives the same results as #18 <-- Logically, because the changed code is exactly the same!!
The diffence between #18 patch and #24 patch is all about tabs and standard coding hence did not pass the test.
Without doubt, it can be set as reviewed.
#26
Committed/pushed to 8.x, thanks!
#27