Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#665790: Redesign the status report page cleaned up the visual output of the Status report page (and the requirements page in the Drupal installer).
Briefly discussing this with @jacine, it turns out that:
- We should use
TH
elements instead ofTD.status-title
. - The icon can possibly be merged into the heading column by using some more fancy CSS (which ensures that a heading [which doesn't fit into the cell and wraps] continues below the heading instead of below the icon).
- It's debatable whether Stark should contain the base styling that ensures a sane width for the heading column and also the background on the heading column. The switch to TH might make this obsolete (for Stark).
Potentially more.
Proposed resolution
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Embed before and after screenshots in the issue summary | Novice | Instructions | Yes |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | Tes |
User interface changes
None
API changes
None
Beta phase evaluation
Issue category | Task because coding standards |
---|---|
Issue priority | Not critical because coding standards |
Unfrozen changes | Unfrozen because it only changes CSS and markup |
Comment | File | Size | Author |
---|---|---|---|
#71 | Screenshot 2015-09-10 12.37.14.jpg | 681.43 KB | LewisNyman |
#69 | interdiff-1591744-63-69.txt | 749 bytes | mansspams |
#69 | clean_up_css_and_markup-1591744-69.patch | 3.52 KB | mansspams |
#63 | Screenshot#63.png | 129.26 KB | pguillard |
#63 | interdiff-1591744-61-63.txt | 1.32 KB | pguillard |
Comments
Comment #1
JacineIn an effort to get a better picture of issues remaining in the HTML5 Initiative, we're removing issues that are not directly HTML5-related. Note: Any issues assigned to the "Markup, CSS and JavaScript" components will still be broadcast on the HTML5 Twitter feed so that interested parties can participate. It might be more accurate to put this in the Markup component. I'll leave that up to you. :)
Comment #2
droplet CreditAttribution: droplet commentedNo reviews please :) I will reroll it after related issue got committed.
Also seems like I can downgrade this patch to D7 now: #1001932: Clean up Status report CSS (and IE fix)
Comment #3
sun#665790: Redesign the status report page landed :)
Comment #4
droplet CreditAttribution: droplet commentedComment #5
droplet CreditAttribution: droplet commentedmyself expatiation and further suggestions :)
remove bg color in stark theme
created an issue to see if it possible to get rid of border from global.
#1601196: remove TH border from base.css
I preferred to remove TR. looking for your comments.
Also, there's one good thing I suggested in #1001932: Clean up Status report CSS (and IE fix) to optimize table rendering performance. (Maybe a new issue to apply it to all possible tables in Drupal)
ref:
http://msdn.microsoft.com/en-us/library/ie/ms531161(v=vs.85).aspx
Comment #6
sun@droplet: Your explanations in #5 are way too sparse for someone like me. ;) It also seems like you forgot to link to that new issue to "get rid of border from global" (whatever that means ;)).
Also, you didn't actually implement any of the suggestions from the OP, or did I miss something? :P ;)
Comment #7
droplet CreditAttribution: droplet commentedno ? or anything I'm missing too ?
It's using TH now
Usability issue, "CSS only" isn't the good way. But I merged it into one column
some css fixes.
Comment #8
mgiffordJust adding a related issue.
Comment #9
droplet CreditAttribution: droplet commentedComment #10
YesCT CreditAttribution: YesCT commentedcorrecting the tag to the more common one.
Comment #11
Aleksandar_P CreditAttribution: Aleksandar_P commentedIcon is merged into the heading column, and the heading is continuing under heading instead of under the icon.
Comment #12
Aleksandar_P CreditAttribution: Aleksandar_P commentedComment #13
YesCT CreditAttribution: YesCT commentedComment #14
YesCT CreditAttribution: YesCT commentedComment #15
Aleksandar_P CreditAttribution: Aleksandar_P commentedHere is the requested screenshot.
Comment #16
mgiffordRemoving tag. Adding image provided by @Aleksandar_P
Comment #17
LewisNymanThis patch is very out of date. I think it's better to start again and see what we can improve. I've uploaded the CSS so it can be reviewed in Dreditor
Comment #18
LewisNymanWe need to SMACSS up these classes, see: https://www.drupal.org/node/2408617
I think we can remove this styling because it is overidden by Seven's table.css. Can we also remove the override in Seven here?
Comment #19
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedLooking into this, what of the old changes we can still implement and if the smacss'ing should be in a follow-up ticket for clarity purposes...
Comment #20
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedAs expected, the last patch (#11) no longer applied, so I re-implemented all the needed changes following the new semantics (so no interdiff needed).
So merged the first 2 td's in one th as suggested, and adjusted the css accordingly. (What will we need for RTL?)
The smacss'ing up of the css has apparently been solved in another ticket? The only thing I should change is the div with class
description
, but I am not sure if this is something we use frequently?Please review! (The use of th now causes a font-weight bold, but not really a fan of resetting this to a normal font weight because it looks better..)
Comment #21
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #22
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #23
LewisNymanThis is looking good, but now we have a row of
<th>
's and a column of<th>
's. Is that valid markup? I ran it through the W3C markup validator and it did complain a little:Can we create a class to apply this to the correct cells instead of the th elements?
These properties are commented out
Comment #24
DeeLay CreditAttribution: DeeLay commentedI'm at DrupalCon LA and I can update the patch to reinstate the commented out code
Comment #25
DeeLay CreditAttribution: DeeLay commentedThe .admin-link css wasn't added in this patch, so I'm guessing best to create a new issue for that?
Un-commented out the rtl css lines and checked the pages look good in LTR and RTL. Attaching new patch and interdiff
Comment #26
DeeLay CreditAttribution: DeeLay commentedAdding screenshots
Comment #27
LewisNymanComment #28
droplet CreditAttribution: droplet commented1 suggestion:
- Adding a class to right column (td)
Comment #29
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedApparently, the status report-page is the only page that uses hidden headers as the only thead, so we should remove it's
<thead>
-tag. or not hide it.I am keen on removing it because the table itself speaks for itself and the labels in the thead don't add anything for anyone in any situation I can think of.
@Lewis: the error the W3C-validator threw was because we had 3 table-elements inside a tr inside the thead and we had 2 table-elements inside a tr inside the tbody.
So the easy fix would be to remove the this:
<th>{{ 'Status'|t }}</th>
But the better solution for me would be to remove the thead altogether and therefor the tbody-tag is also no longer needed. (the th-elements provide more than enough context & information)
Extras:
1) The RTL-styling was in comments because I wasn't sure how this should be on rtl-installs, but the screenshots made this more clear.
2) I was bothered by the fact that tr's without icons also had padding for the icon, so I fixed this. => This causes some wanted visual changes, so any opniion on this would be great.
3) Added a extra if statement to status-report.html.twig so the div with the status information is not printed for th's that don't need it.
4) @Droplet, wasn't sure why we should add a class on the td?
Comment #30
LewisNymanOk cool, this is looking good. I'm not sure what the effects of removing the th's will have on screenreaders, so tagging this for accessibility review.
I wonder if there is another class we could use instead of colored? I know that there is a color class being added but it would nice to keep the other classes logical, instead of presentational.
Comment #31
LewisNymanComment #32
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented@LewisNyman not really sure if this is the tag you wanted?
Every
<tr>
now has the classes:system-status-report__entry--{{ requirement.severity_status }}
This way, the classes are more consistent and more readable. The tr with the classsystem-status-report__entry--info
is now the exception instead of the other 2 posiibilities in warnings.Adjusted the css so nothing visually changes in comparison with last patch.
The interdiff is a bit chaotic because of tab-indents being altered :)
Comment #33
LewisNymanOk thanks. This looks great I found one suggestion:
If we just set padding-left here then we don't have to override the vertical padding, which is nice.
Screenshot and bet eval added. I'll ping someone who can review the accessibility.
Comment #34
Manjit.SinghComment #35
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedGood Work all, #34 looks good.
it is removing
+ padding-right: 40px;
as it was suggested by @LewisNymanMoving this to RTBC as of now.
Comment #36
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedSupplied a interdiff for #34.
This will still need some accessibility-review before it can go on rtbc.
Comment #37
idebr CreditAttribution: idebr commentedThe padding-left is not applied for RTL. This results in the icon being displayed op top of the title.
Re: #35
Lewis was suggesting only styling the padding-left instead of all the padding. The padding-right still needs to be applied for RTL.
Comment #38
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commented@LewisNyman:
Not sure why you suggested to not override padding-right on RTL?
As screenshot shows, the padding stays necessary:
So everything inside #32 Is ok and there should the accessibility review should happen?
Comment #39
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedExtra: is the same interface being used on the install guide of Drupal? If so, it would be great if we can have some screenshots on those install pages, so we are sure everything looks ok there.
Comment #40
LewisNyman@MathieuSpil We should override the padding on RTL. I guess I worded it badly.
We only need to set padding-left here, we don't need to set padding on the right, top, and bottom.
Ah good point, we should.
Comment #41
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedComment #42
MathieuSpil CreditAttribution: MathieuSpil as a volunteer commentedUpdated css based upon #32, so it now only overrides padding-left on LTR and padding-right on RTL.
Tested it on the install steps, and it broke indeed.
I assumes there were 3 possible classes of the tr's:
-
.system-status-report__status-icon--error
=> red-
.system-status-report__status-icon--warning
=> yellow-
.system-status-report__status-icon--error
=> defaultBut appareantly there is one more:
-
.system-status-report__status-icon--ok
=> only used on the install pages as far as I can see.As defined in:
So tweaked the patch so now the warning and error are the exception. => CSS looks now more logical, while the twig looks a bit more complex (moved the if-statement one line higher)
After that took some screenshots, who do we address for accessibility?
Comment #43
LewisNymanAh sorry. Needs a reroll
Comment #44
pguillard CreditAttribution: pguillard commentedJust rerolled patch #42
Comment #45
cchanana CreditAttribution: cchanana commentedComment #46
cchanana CreditAttribution: cchanana commentedAfter applying patch, design is looking same. Please find the attach below and after patch screenshots.
Comment #47
googletorp CreditAttribution: googletorp as a volunteer commentedComment #48
googletorp CreditAttribution: googletorp as a volunteer commentedThis patch looks good to me. Since it only changes CSS and HTML it's an unfrozen change, so should be fine for beta.
Before and after pics located in #46
Comment #49
alexpottI don't think this
if
is the way to go here. The whole --has-icon looks like something we could do without.Comment #50
LewisNymanNice! Alex is right, we can just add icon classes to the th. One less div! I removed the requirement for the accessibiltiy review because the elements used haven't changed.
Comment #51
lauriiiIs this visual change approved?
Before:
After:
Comment #52
LewisNymanComment #53
pguillard CreditAttribution: pguillard commented@LewisNyman : What is the remaining work to do, as you pushed Needs Work ?
Comment #54
pguillard CreditAttribution: pguillard commentedI don't see any visual change, so I try Needs Review again
Comment #55
LewisNyman@pguillard Sorry, the first column of the messages when warning or error are now bold, they weren't bold before. We shouldn't make any visual changes in this issue.
Comment #56
pguillard CreditAttribution: pguillard commentedThanks @LewisNyman
I replaced back
<th>
with the original<td>
Comment #57
LewisNyman@pguillard Thanks, I think we're close with this. I think we need to keep the
<th>
for accessibility reasons. Is there a way we could just style the th so it doesn't include the bold styling?Comment #58
rudraram CreditAttribution: rudraram at Axelerant commented@LewisNyman Just re-rolled #56 as per your suggestion. Please see if that is correct. Attaching screenshot.
Comment #59
rudraram CreditAttribution: rudraram at Axelerant commentedComment #61
rudraram CreditAttribution: rudraram at Axelerant commentedOhk. My patch in #58 didn't capture everything properly. I updated the patch. Apologies for that. Screenshot remains the same as #58 though.
Comment #62
rudraram CreditAttribution: rudraram at Axelerant commentedComment #63
pguillard CreditAttribution: pguillard commentedChecking #61 in order to set it to RTBC, I discovered the patch was not applying anymore, just rerolled it.
Screenshot remains the same as #58.
Comment #64
LewisNymanGreat, thanks.
Comment #65
alexpottThe alignment of the text and icons has changed - the icons used to be in a "column" of their own.
Comment #66
alexpottTo see what I mean look at #51 thanks @lauriii and @emma
Comment #67
mansspams CreditAttribution: mansspams at Wunder commentedOk, will do.
Comment #68
emma.mariaI can help to visually test the solution when it is ready also :-)
Comment #69
mansspams CreditAttribution: mansspams at Wunder commentedHere is a update.
Comment #70
mansspams CreditAttribution: mansspams at Wunder commentedComment #71
LewisNymanThanks, this looks better.
Comment #72
alexpottNeeds LTR thingy. Will add on commit.
Comment #73
mansspams CreditAttribution: mansspams at Wunder commentedThere is one already.
Comment #74
alexpottCSS and Twig only - permitted in beta. Committed eb6330d and pushed to 8.0.x. Thanks!
@mansspams this is what I meant...
Fixed on commit.