Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The styling of the admin-panel
is not inline with our CSS coding standards.
Proposed resolution
Bring them inline with our standards.
Remaining tasks
Review the current CSS — What to look for when reviewing CSS
User interface changes
None
API changes
None
Beta phase evaluation
Issue category | Task because coding standards |
---|---|
Unfrozen changes | Unfrozen because it only changes HTML/CSS |
Comment | File | Size | Author |
---|---|---|---|
#82 | 2399939-82.patch | 6.08 KB | idebr |
#82 | interdiff-82-75.txt | 1002 bytes | idebr |
#82 | 2399939-82-after.png | 255.54 KB | idebr |
#82 | 2399939-82-before.png | 212.6 KB | idebr |
#77 | Screen Shot 2015-02-27 at 3.58.17 PM.png | 98.45 KB | idebr |
Comments
Comment #1
LewisNyman CreditAttribution: LewisNyman commentedI'm not even sure if
admin-panel
is the correct name here, components should be generic irrespective of their content.Comment #2
DickJohnson CreditAttribution: DickJohnson commentedBy template name and comments it's been called as admin blocks, but I'm not sure if that's a correct name either as technically these things are not drupal blocks.
Comment #3
DickJohnson CreditAttribution: DickJohnson commentedCurrent css on modules/system/css/system.admin.css is relatively good for this particular element. There's only few rows:
Rest of the stuff is done either in Seven or in Bartik. I'd like to change the template so that we would have some reusable components for Seven and Bartik. The template could look something like:
modules/system/templates/admin-block-html.twig
I'm not completely sure if this is appropriate or should that be done both in Seven and in Bartik. But f.ex for Bartik it would change the look of css to:
Which is starting to look relatively good. dt and dd are still a bit of a problem but I'm not sure if there's much we can do about it.
Comment #4
DickJohnson CreditAttribution: DickJohnson commentedSo, I think is the only way to get feedback.
Comment #5
idebr CreditAttribution: idebr commentedThe css looks spot on, a sight for sore eyes :)
Are you leaving the original
body
class in for compatibility? A pure SMACCS implementation would suggest justadmin-panel__body
instead ofbody admin-panel__body
.Comment #6
LewisNyman CreditAttribution: LewisNyman commentedYep I think we should get rid of the body and description classes.
Apart from that, it looks good. I manually tested the patch in Classy, Seven, and Bartik. It looks the same.
Comment #7
DickJohnson CreditAttribution: DickJohnson commentedRemoved body and description -classes.
Comment #8
idebr CreditAttribution: idebr commentedThe component looks much cleaner now :)
I found one difference in Seven, since Seven has a custom template for admin-block-content.html.twig that needs to be updated with the new class.
Screenshot before:
Screenshot after:
Comment #9
LewisNyman CreditAttribution: LewisNyman commentedOh nice catch. This is confusing. The description class isn't part of the admin-panel component, it's part of the admin-list component. Why do we have two components? I guess it doesn't hurt...
What we can do is move the selector into the admin-list.css file and rename admin-panel to admin-list. Then it fixes the minor regression and puts it where it belongs. I also renamed the dt and dd selectors in Bartik to match this.
I also went through and found some CSS that wasn't being used, and tidied up a few properties.
Comment #10
DickJohnson CreditAttribution: DickJohnson commentedTwo components doesn't really hurt on this case as another one is for content and another one for the rest. Crappy thing is that by name no-one knows what admin-panel and admin-list are. I'd rename those as admin-block and admin-block__content.
Comment #11
LewisNyman CreditAttribution: LewisNyman commentedAdmin block sounds just as generic as admin panel. Panel at least is a convention: Bootstrap, Foundation
Comment #12
LewisNyman CreditAttribution: LewisNyman commentedI have problems with it being called
admin-*
though. What's do adminy about it? We don't name another of our other UI components like this.Comment #13
idebr CreditAttribution: idebr commentedI agree with @LewisNyman the
admin-*
part seems superfluous. Let's take a step back and see if we can split this into seperate components.Candidate names per component:
- panel (Bootstrap / Foundation)
- list-group (Bootstrap) / block-list (Inuit)
- list-group-item (Boostrap) / complex-link (Inuit) / link-group (idebr)
Current hierarchy:
Suggested hierarchy:
Current markup:
Suggested markup:
Comment #14
LewisNyman CreditAttribution: LewisNyman commentedList group is a little generic for my tastes, I just noticed the Seven style guide defines it as nav list. Will post more detailed feedback in a bit. We also have a separate issue somewhere for creating a reusable 'panel' component. #2113903: Introduce a 'pane' component to visually group form items
Comment #15
LewisNyman CreditAttribution: LewisNyman commentedI think
panel__content
instead ofpanel__body
would fit closer to the block terminology we use, what do you think?I noticed we are switching to three components from two. It feels like it could be too much, here's a proposal with two components. This fits with my interpretation of SMACSS, which doesn't consider on HTML structure.
Comment #16
DickJohnson CreditAttribution: DickJohnson commentedI'm fine with that one.
Comment #17
idebr CreditAttribution: idebr commented@LewisNyman
Agreed
In #15 you suggest
.list-group
for the<ul>
, did you change your mind? Either is fine by me.Indeed I suggested to use 3 components instead of 2:
.panel
(I think this one is uncontested).list-group
.link-group
If we define
__
as a child, then in a pure implementation the class names should be:This makes the classes very verbose.
Note: we also need a class on the
<a>
for the css that is currently being applied to.admin-list li a
if we wish to keep the styling seperated from the markup.I based the maximum of 2 levels of nesting on the Sass modules styleguide: https://gist.github.com/trinonsense/8295577#module-component:
One could argue either way, but two components could work just as well. @LewisNyman, I'll leave this one up to you and work on a patch when the decision is in. Either way this case should be document on the CSS coding standards page for later reference: https://www.drupal.org/node/1887918
Comment #18
LewisNyman CreditAttribution: LewisNyman commentedBut we don't need to mimic the nesting of HTML in our component selectors, so
list-group__item__link__label
andlist-group__item__link__description
can just belist-group__item__label
andlist-group__item__description
. Does that make sense?Comment #19
DickJohnson CreditAttribution: DickJohnson commentedStarted to work on this. What still needs to be done:
1. Split the {{ item.link }} up in admin-block-content.html.twig
2. Implement the changes to Seven
3. Fix .list-group in Bartik
4. Rename the files?
Comment #20
DickJohnson CreditAttribution: DickJohnson commentedImplemented stuff to seven.
Remaining things to do:
1. Split the {{ item.link }} up in admin-block-content.html.twig
2. Fix .list-group in Bartik and Seven
3. Rename the files?
Comment #21
DickJohnson CreditAttribution: DickJohnson commentedCurrent markup by idebr on #13 is not actually current markup from system module, it's current markup from Seven theme. Current markup from system.module outputs the item.link as
<a href="foo/bar">Blaa</a>
without any spans.Comment #22
LewisNyman CreditAttribution: LewisNyman commentedThat's fine, let's just use those sub-items in Seven but don't use it elsewhere
Comment #23
DickJohnson CreditAttribution: DickJohnson commentedFinalized the template refactoring. Decided to split the {{ item.link }} anyways, as I don't think it's good idea to output mixes of links and labels. Didn't go with suggested markup as it would have changed the functionality a bit. So ended up with:
Atleast Bartik's admin panel is looking pretty ok after this:
Comment #24
DickJohnson CreditAttribution: DickJohnson commentedRight. 2 mins after sending the patch I can see that there's one false class on link.
Comment #25
DickJohnson CreditAttribution: DickJohnson commentedFixed it.
Comment #28
DickJohnson CreditAttribution: DickJohnson commentedAnd once more:
Comment #31
DickJohnson CreditAttribution: DickJohnson commentedAfter a short chat in irc with @lewisnyman reverting back to {{ item.link }}.
Things still to do:
1. On Bartik theme ".region content ul" and "block ul" are overriding margins on .list-group. Everything else seems to be fine. Those could be handled on Bartik cleanup issues also.
Comment #32
LewisNyman CreditAttribution: LewisNyman commentedThis margin does nothing, we can delete it.
I think we can remove the final unit and change this to padding: 5px 5px 15px;
These should be changed back to dl, dt, and dd. I think they are like that doe accessibility reasons
We can just name this class list-group__description. See the best practices - https://www.drupal.org/node/1887918#sub-components
Comment #33
DickJohnson CreditAttribution: DickJohnson commentedMade the changes suggested by @lewisnyman on #32.
Comment #34
DickJohnson CreditAttribution: DickJohnson commentedComment #35
LewisNyman CreditAttribution: LewisNyman commentedWe are getting there! Thanks for the hard work.
This css used to be selecting the dt, but now it's selecting a different element, because the dt now has the class panel__link, not .panel__content
I think we can delete this CSS, padding iss 0 by default anyway so we don't need that. I don't know what clear left does here. Let's try deleting it and seeing what comes up
This class no longer exists
Comment #36
LewisNyman CreditAttribution: LewisNyman commentedComment #37
DickJohnson CreditAttribution: DickJohnson commentedOk, the reason why I didn't notice #35.1 is that it's not actually doing anything difference if it's in .panel__content, .list-group__link or if it doesn't exist, so I removed it.
2 and 3 are fixed.
Comment #38
DickJohnson CreditAttribution: DickJohnson commentedComment #39
LewisNyman CreditAttribution: LewisNyman commentedComment #40
idebr CreditAttribution: idebr commentedComment #41
DickJohnson CreditAttribution: DickJohnson commentedI'm not touching Seven on this patch at all as it's handling it's own admin-panel structure on template level.
Comment #42
LewisNyman CreditAttribution: LewisNyman commentedI'm happy to spin out Seven into a separate issue.
We should update this comment. How about:
"Panel.
Used to visually group items together."
Same situation here, in fact we should rename this CSS file to panel.css.
I think we can leave moving around the CSS files in Bartik for #2398453: Clean up the "admin" component in Bartik
Comment #43
DickJohnson CreditAttribution: DickJohnson commentedSeven's admin-panel.css was missing file-comment completely so decided to do it that way. 42.1 fixed as suggested.
Comment #44
LewisNyman CreditAttribution: LewisNyman commentedThanks, the only thing left to do here is to rename the Seven file from admin-panel.css to panel.css
Comment #45
DickJohnson CreditAttribution: DickJohnson commentedRenamed.
Comment #46
brahmjeet789 CreditAttribution: brahmjeet789 commentedChanged the admin-pannel.css o pannel.css
Comment #47
LewisNyman CreditAttribution: LewisNyman commented@brahmjeet789 I think you might of uploaded the wrong patch?
The patch in #45 looks good though. It needs a reroll now though :(
Comment #48
brahmjeet789 CreditAttribution: brahmjeet789 commentedThanks @LewisNyman, yup the patch #45 is working fine .
Comment #49
DickJohnson CreditAttribution: DickJohnson commentedDid a manual reroll due #2399709: Remove media.css from Bartik, add it's current code directly to the components it references.
Comment #50
idebr CreditAttribution: idebr commentedSeven has its own implementation for the panel content, but the switch from .admin-panel to .panel does affect Seven. For comparison:
Comment #51
DickJohnson CreditAttribution: DickJohnson commentedIt seems that I forgot to apply new classes to Sevens panel.css on reroll.
Comment #52
DickJohnson CreditAttribution: DickJohnson commentedThat should do it.
Comment #53
LewisNyman CreditAttribution: LewisNyman commentedComment #54
LewisNyman CreditAttribution: LewisNyman commentedWe don't need margin 0 here.
We also don't need this last 5px unit here.
Comment #55
DickJohnson CreditAttribution: DickJohnson commentedFixed #54.1 and #54.2.
Comment #56
idebr CreditAttribution: idebr commentedThis comment should be updated to reflect the new name of the component. I suppose it could be the same as the system.admin.css. This file should also include an @file docblock.
This css was removed after 35.2, but the change was lost somewhere along the way. This can still be removed.
Comment #57
DickJohnson CreditAttribution: DickJohnson commentedFixed #56.1 and #56.2.
Comment #58
DickJohnson CreditAttribution: DickJohnson commentedComment #59
idebr CreditAttribution: idebr commentedThere is a mismatch in the new class for Bartik. Previously the styling was applied to the
<dt>
, while in the patch the styling is applied to the<dl>
. This introduces a visual regression.This styling was historically applied to the
<dd>
tag, but currently does not match any markup. Let's correct this styling to it applies again.Comment #60
DickJohnson CreditAttribution: DickJohnson commentedFixed the issues mentioned in #59.
Before:
After:
Comment #61
DickJohnson CreditAttribution: DickJohnson commentedComment #62
LewisNyman CreditAttribution: LewisNyman commentedThanks for the hard work here, this is a big improvement.
All that is left to do is to move these components out of admin.css and into panel.css and list-group.css
Comment #63
mherchelWorking on this now in Bogota
Comment #64
mherchelPatch and interdiff attached!
Comment #65
DickJohnson CreditAttribution: DickJohnson commentedLooks like you forgot to add Sevens panel.css and Bartiks list-group.css file.
Comment #66
mherchelYep. Fixing now :)
Comment #67
mherchelFixed
Comment #68
DickJohnson CreditAttribution: DickJohnson commentedEverything else seems to be okay but in:
1. bartik/css/components/admin.css
That's somehow wrong. Should be in panel.css, right?
2. Seven's panel.css is stil missing.
Comment #69
DickJohnson CreditAttribution: DickJohnson commentedComment #70
idebr CreditAttribution: idebr commentedSevens panel.css is still missing
Some code from bartiks button.css seems to be leaking
Comment #71
DickJohnson CreditAttribution: DickJohnson commentedbutton.css is removed from bottom of libraries.yml to upper parts of it yes. That might cause possible problems.
Comment #73
LewisNyman CreditAttribution: LewisNyman commentedI gave the patch another go
Comment #74
DickJohnson CreditAttribution: DickJohnson commentedAre these unnesessary?
Comment #75
LewisNyman CreditAttribution: LewisNyman commentedHere are before/after screenshots of the panels in Bartik, you tell me ;-)
Also, the patch before was missing a blank line in a file. Here is the fixed patch.
Comment #76
DickJohnson CreditAttribution: DickJohnson commentedChecked the patch and it's great.
Comment #77
idebr CreditAttribution: idebr commented@LewisNyman In 59.2 and 59.3 I suggested to fix the Bartik admin-panel selectors instead of removing the styles, since they historically created a much nicer admin-panel. For reference a screenshot of the Bartik admin-panel in Drupal 7:
Comment #78
LewisNyman CreditAttribution: LewisNyman commented@idebr Ah yes, sorry I missed that/
Comment #79
DickJohnson CreditAttribution: DickJohnson commentedI think it should be handled it follow-up issue. We haven't done any stylistical changes in these refactor/write css per standards issues. It is a valid point anyways.
Comment #80
DickJohnson CreditAttribution: DickJohnson commentedComment #81
idebr CreditAttribution: idebr commentedActually, in the case of a broken selector we historically fixed the selector instead of removing the styling, for example:
- #1663210: Clean up css in the User module
- #2398461: Clean up the "comments" component in Bartik
With our current infrastructure it is very easy to introduce regressions without anyone noticing. When this happens we should fix it sooner rather than later and certainly not remove any styling that applied previously, since it is very likely we will lose track of it entirely.
I understand you would like to get this committed, so I'll work on a patch to bring the visual styling back to the admin panels.
Comment #82
idebr CreditAttribution: idebr commentedThe missing styling was actually already removed; it used to live in system.admin.css in Drupal 7. I copied the styling over from Drupal 7 to fix the visual regression.
The styling that was removed by @LewisNyman in #75 was actually never used, not even in Drupal 7.
Screenshot before:
Screenshot after:
Comment #83
idebr CreditAttribution: idebr commentedComment #84
DickJohnson CreditAttribution: DickJohnson commentedChecked the patch, it's applying and fixing the coding standard issues. By what @idebr pointed out in #81 setting this as RTBC even it's changing the look of Bartik's panel a lot.
Comment #85
webchickHm. I'm not 100% sure of this change. Aren't we worried about namespace collisions with Panels module in contrib?
Comment #86
idebr CreditAttribution: idebr commentedI checked the current Panels 8.x-3.x codebase for naming conflicts. Panels prefixes its class names with
.panels-*
and an individual panel is called a.panels-pane
. There is no direct naming conflict and even the namespace is available.Arguments for
.panel
:Arguments against
.panel
:I think we're safe on against #2, since .panel is used on a very specific set of pages that are not commonly extended with Panel (contrib) content. I'm not sure on against#1, perhaps someone who actually uses Panels can comment on it?
Comment #87
LewisNyman CreditAttribution: LewisNyman commented@idebr Thanks that was a more detailed answer than I would of given :). I don't think argument #1 against panel is a concern, you could say that about any class really, the process would be the same: remove or override existing system CSS.
Comment #88
alexpottSo @idebr's comment in #8 will be addressed in a followup? Has one been created?
Comment #89
idebr CreditAttribution: idebr commented@alexpott The selector collision has been in HEAD since Drupal 7. Now that we have proper components, I'm not sure if this collision was intended or not. I have created a followup and postponed it on this issue so we can keep moving forward: #2449555: Check design for panels (admin/config) with the Seven style guide
Comment #90
alexpottCSS is not frozen in beta. Committed c016165 and pushed to 8.0.x. Thanks!
Comment #93
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and commentedComment #94
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commented