Closed (duplicate)
Project:
Drupal core
Version:
8.3.x-dev
Component:
simpletest.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
6 May 2015 at 12:26 UTC
Updated:
7 Jul 2017 at 03:37 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
lewisnymanCSSlint errors from http://lewisnyman.co.uk/drupalcore-frontend-toolkit/
Comment #2
mirakolous commentedHere is a patch to clean up css errors
Comment #3
mirakolous commentedComment #4
poojakural commentedComment #5
poojakural commentedI have tested it on csslint. it is cleaned now. Now no error found. Passed this patch "2483913-2.patch"
Comment #6
lewisnymanThis still includes the
thin the selectorI think we can delete this CSS completely, the background-color: white; doesn't override anything, we shouldn't be changing the text color in modules either
We are still prefixing this class with
tr. Do we need that?I wonder what this does as it's not a simpletest class, where is the message class used?
Comment #7
rudraram commentedUpdated Mirakolous's patch as per the above suggestions from LewisNyman.
Comment #8
lewisnymanComment #9
rudraram commentedThanks @LewisNyman. I just returned realising I forgot to change the status.
Comment #10
poojakural commentedComment #11
poojakural commentedComment #12
poojakural commentedTested: Implemented suggested code changes and no error on css lint.
Passed
Comment #13
lewisnymanI think we need to provide some before/after screenshots to show that we haven't caused any visual regressions in the UI. We've changed the CSS but we haven't changed any markup, so I'm not sure all this CSS is applying correctly.
Comment #14
rudraram commentedMy patch in #7 gave a different output when I tested and is caused by this
A yellow background for the td inside .selected class is getting applied.
Screenshot attached.
Updating the patch with this change.
Comment #15
rudraram commentedComment #16
rudraram commentedUpdated patch fixing the above issue and attached before after screenshots.
Before
After
Comment #17
lewisnymanThere are loads more improvements we can make to this CSS.
Can we update this and the markup so we use CSS background images and SVGs instead?
It looks like this CSS no longer applies, we can change it to:
Can we put padding on the class instead of on the label element? Like this:
Also there is no RTL styling for this.
We can delete this, I disabled it and it had no effect.
I think we can definitely remove the ID here, but maybe we can remove the td as well? That would be nice.
What does this do? I think we can delete it.
Delete this, modules should be messing with global UI components
I don't think we should mess with the colors here but we should at least remove the odd/even colors and replace with CSS nth-child
Comment #18
pjbaertComment #19
pjbaertun-assigning this again. I'll try to pick this up tomorrow.
Comment #20
simply021 commentedRegarding #17 is almost everything done except changing markup for SVG images.
Comment #21
barbarae commentedbarbarae: I am doing the screen captures before and after this current patch was applied.
to do this:
1. click on the simplytest.me button
2. clear the patch to be applied (i.e. delete the patch name)
3. after module is installed do the following steps
a. click on
b. find the testing module and turn it on (click the box to the left of the name)
c. save the change (at the bottom of the screen)
d. click on in the tool bar
e. find the test button and click (execute)
f. the resulting screen image is the "before" image to capture. Expand a couple items by clicking on the "<"
4. capture an image of the "before" resulting screen page
5. Do the same thing again, leaving the patch enabled (do not erase it). Do steps a. through f. above again
6. capture an image of the "after" resulting screen page.
7. Be sure the patch did not break anything, and observe the changed image.
Question: Should we have run all the actual tests from simpletest on the BEFORE and then do the same AFTER to show there are the same pass/fail resulting counts?
Comment #22
lewisnymanI had a chat with alexpott about this UI. Instead of using simpletest's own colors and classes we can reuse the color classes in Seven's colors.css.
This is Alex's description of how they map:
This also means we would need to add the color-success class as a hover override in Seven's tables.css:
Comment #23
manjit.singhComment #24
manjit.singhrerolled a patch and added a class
.color-successintables.csson hover and focus.@lewis I have added a color according to the style guide of Seven. https://groups.drupal.org/node/283223
Comment #25
lewisnyman@Manjit.Singh Great stuff. It looks like we already have the background color set in
color.cssand it's different. The aim here is to make sure the color doesn't change:It also means we should be able to delete all the simpletest-pass/fail/debug CSS. Yay!
Comment #26
maninders commentedComment #27
maninders commentedWe have colors already mention in
colors.cssas you have mention in #25. so can we remove colors from simpletest.module.css.Please confirm.
And some csslint warnings coming on these files
colors.cssandsimpletest.module.cssso we have to remove that also?
tr.simpletest-pass,
.simpletest-pass:nth-child(odd).odd {
background-color: #b6ffb6;
}
.simpletest-pass:nth-child(even).even {
background-color: #9bff9b;
}
tr.simpletest-fail,
.simpletest-fail:nth-child(odd).odd {
background-color: #ffc9c9;
}
.simpletest-fail:nth-child(even).even {
background-color: #ffacac;
}
tr.simpletest-exception,
.simpletest-exception:nth-child(odd).odd {
background-color: #f4ea71;
}
.simpletest-exception:nth-child(even).even {
background-color: #f5e742;
}
tr.simpletest-debug,
.simpletest-debug:nth-child(odd).odd {
background-color: #eee;
}
.simpletest-debug:nth-child(even).even {
background-color: #fff;
}
Please confirm.
Comment #28
snetcher commented#simpletest-form-table .select-all
This style does not make sense, since, in the file /core/themes/seven/css/components/tables.css - the width of this cell is already set to 1px, and as you know, the minimum width of the table cell is equal to the maximum width of a nested objects
Comment #30
rutgers03 commentedRemoved the needs screenshot because it seems that all the requests were full filled.
Comment #31
hog commentedChecked code with csslint, have only one warning.
csslint: There are 1 problems in /var/www/drupal8-contrib/core/themes/seven/css/components/tables.css.
tables.css
1: warning at line 126, col 1
Element (th.select-all) is overqualified, just use .select-all without element name.
th.select-all {
Comment #35
mile23We're currently using a rule-by-rule approach to CSS cleanup for consistency: #2865971: Use stylelint as opposed to csslint in core