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
A CSS regression was introduced to resolve a critical #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()
Before #2550981 issue was fixed.
After Fixed #2550981 issue
Proposed resolution
Fix CSS on UL in the error messages. Probably fine to keep it on a new line just doesn't need the indent.
With latest patch in #13
Remaining tasks
User interface changes
Fix CSS regression.
API changes
Data model changes
Follow-up to #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()
Comment | File | Size | Author |
---|---|---|---|
#60 | fix_inline_list_css-2557367-60.patch | 11.51 KB | davidhernandez |
#50 | Fix-inline-list-CSS-2557367-50.patch | 11.91 KB | Studiographene |
#49 | Fix-inline-list-CSS-2557367-49.patch | 11.91 KB | nikky171091 |
#44 | fix_inline_list_css-2557367-44.patch | 12.45 KB | madhavvyas |
#40 | fix_inline_list_css-2557367-40.patch | 13.14 KB | emma.maria |
Comments
Comment #2
Wim LeersNo patch yet, so think the RTBC here was accidental? :)
Comment #3
joelpittetThanks @Wim Leers, cloning abnormality:P
Comment #4
alexpottHere's a patch cut from the latest changes #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages()
Comment #5
alexpottComment #6
star-szrThanks!
Comment #7
Wim LeersLet me repeat my concern/comment from the original issue:
Comment #8
joelpittetComment #9
joelpittetA note on whitespace modifiers in Twig. Control and comment statements remove the newlines after themselves so you only need them for spaces in most cases.
Show and tell: http://twigfiddle.com/5aiuow
Comment #10
joelpittet@Wim Leers can you elaborate on what #7 means here. A bit confused as to what to do with that info.
Comment #11
joelpittetHere's a touch-up of the templates with an attribute object to hang those classes on(we usually try to add attributes using that object), also cleaned up the whitespace modifiers some and added them to core template as well (may break those tests).
Removed the overqualified selector (div) and added a class as a modifier. Likely needs someone to review the CSS selector for the nested list inside the wrapper, but I agree in general it will be much easier to style the component from the outside (as we can see it's hard to style the div that contains that inline class).
Comment #13
joelpittetDid break those tests, huzzah. Hopefully this fixes them.
Comment #14
xjmPart of this got added in #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages(), so we need to reroll the patch on top of that.
Both @davidhernandez and @alexpott had a couple concerns in the other issue.
Once we have some consensus on this issue and the fixes (whatever they may be) are committed, we should do a couple things:
|safe_join
with commas, and convert them to the inline lists too. (Needs followup.)Comment #15
xjmComment #16
xjmFiled #2559793: Convert templates and inline templates using |safe_join to inline item list.
Comment #17
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedRe-rolled not required. Latest patch tried in Drupal 8.0.x branch after rebase It works for me.
Comment #18
joelpittet@xjm I wonder how much of that needs to be done here? Although a few of those items I believe are resolved by the proposed patch.
The styles are moved to classy.
That was also resolved here as well I believe.
I went with it should show up inline, but with the class on the wrapper that is now a one line CSS change.
That may be out of scope of this issue but it doesn't remove them it just doesn't explicitly style them different from other item lists.
I may have misread this but I removed the broken twig template in head with the whitespace fixes in this patch.
Comment #19
xjm@madhavvyas, reroll means exactly rebasing with the patch applied and resubmitting the updated patch on the issue. :)
@joelpittet, honestly I'm not entirely sure; @alexpott and @davidhernandez were the ones with concerns (see the other issue).
Comment #20
xjmAlso, we should probably manually test this again once it's rerolled... the screenshots in the summary are not current.
Comment #21
xjmOh also:
Let's manually test with those too and add screenshots of what that looks like. I think it should be in scope for this issue to make sure they don't look stupid.
Comment #22
joelpittet@xjm It doesn't need a re-roll I tested it too, both git apply and patch -p1 work. What command are you using that is saying it needs a re-roll?
I was discussing on IRC with @davidhernandez as I made the changes to ensure he was cool with them, but could get his opinion FTR.
Comment #23
joelpittetUpdated the screenshots with latest from head in the IS
Before the patch
With latest patch in #13
Comment #24
davidhernandezI don't think we actually need to do this now. I'll try to take a look later today. A lot of this can be simplified now that we are moving the CSS to Classy.
Comment #25
xjmThanks @joelpittet.
Can we test with the other features of item lists like title and description and empty to see what it looks like?
Comment #26
davidhernandezI still argue the inlining is the responsibility of messages to set since it is the outer wrapper, not the inline list, but I won't dwell on it.
I was able to refactor some (I had to stop myself from going down the rabbit hole. Is it too late to refactor all of core's CSS?) This allowed some simplifying of Bartik's override and completely removing what was added in Seven. Also, deleting a number of other declarations. Yay!
I'm ok with this. It should be the most flexibly overridable. I checked the form errors (Thanks to stefan.r's awesome little module.) and the lists on the Views list page. I checked all four themes LTR and RTL. Didn't make screenshots though. It's late here.
Comment #27
joelpittetI'm OK with the changes:) Needs screenshots as said in #25
Comment #28
davidhernandezHere is left to right. I can do right to left later. ...but don't let that stop anyone else from doing it. ;)
something something multiple file upload would be awesome.
Comment #29
davidhernandezright to left
Skitch and Stefan's module have made this so much easier to do.
Comment #30
alexpottSo what are we going to do about the title support?
<h3>
tags don't play nicely with inlineness.Comment #31
davidhernandezI dunno. Do we have a usecase for that yet so it can be thought out?
I just realized there is css for .title. Why is that there? Did we remove that accidentally? We don't actually need it with the h3, but I don't know if that is just legacy.
Comment #32
alexpottI don't think there is a use-case for it with a comma list but an inline div having h3 inside doesn't seem to make much sense - and it is possible to do with the current template.
Comment #33
joelpittetWe could for completion sake just inline the the h3 as well and tack on a colon?
Comment #34
davidhernandezThe item list title is an H3. I don't think it should be inline, it is not a label.
I hard-coded it real quick to see how it looks. It seems fine to me. I wouldn't expect it to, out-of-the-box, be treated any different. It's an H3,
Comment #35
joelpittet#34 confirmed it is possible to use the h3 with inline lists though a very unlikely use-case and it doesn't look bad. Also very easy to override with CSS or override the template to make this work for uses cases.
Feel free to disagree but I think this is a nice fix that makes working with with this template much easier for themers, allowing them to target the wrapper container and it's child elements.
Comment #36
xjmThis line of code makes my brain bendy.
Also, a few lines down, we have:
Hash or no hash?! I confused. There may be a perfectly logical explanation for this, but that's what made me hesitate before committing.
Comment #37
xjmForgot to say, I'm okay with the title thing if others are. If you put an item list with a title in a sentence, well that is your silly. Thanks for the screenshots!
Comment #38
joelpittet#wrapper_attributes
are something @sun came up with I believe to let render elements pass around things. It is meant to move the attributes on to the<li>
tag, as the wrapper is of each item's value is the the<li>
and it can sometimes take render array. Same pattern for#type => table
.$variables['wrapper_attributes'] = new Attribute($variables['wrapper_attributes']);
With all our other attributes we do this. I'd prefer to just instantiate an empty
Attribute()
in the hook_theme(). But that is untested approach.These ones are "magically" done for all templates, and I tried to remove that magic #2108771: Remove special cased title_attributes and content_attributes for Attribute creation:
Comment #40
emma.mariaI rerolled the patch.
Comment #41
joelpittetThank you emma. I diffed the diffs and the only move was the components are on component instead of theme.
Comment #43
davidhernandezComment #44
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedPatch rerolled #40
Comment #45
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedComment #48
davidhernandez@madhavvyas, can you check your reroll? This shouldn't be there. This code above was committed the other day so it should already be in head.
Comment #49
nikky171091 CreditAttribution: nikky171091 commentedNew Rerolled patch!
Comment #50
Studiographene CreditAttribution: Studiographene commentedRerolled!
Comment #51
madhavvyas CreditAttribution: madhavvyas as a volunteer commentedComment #54
madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commented@davidhernandez When I check in HEAD, I do not found theme code as you specified
/core/modules/system/system.libraries.yml
Comment #55
davidhernandezDid you rebase your branch with 8.0.x? Those CSS file moves, and the change to system.libraries.yml, were done on Sunday. And if you look at the patch in #40 and all the ones before, you'll see the code is not there.
Comment #58
madhavvyas CreditAttribution: madhavvyas as a volunteer and at CIGNEX commentedI have rechecked. I have rebased my 8.0.x branch using
git pull --rebase
I do not found changes specified in comment #48
@davidhernandez Please advice if you referring any other branch.
Comment #59
davidhernandez@madhavvyas the patch you uploaded in #44 had that code. It should not have been there. Please reroll from #26 or #40, not the patches after.
Comment #60
davidhernandezLet's see if this one works.
Comment #61
RainbowArrayJust finished a line by line comparison of the last RTBC patch with the newest patch. The only difference is that the old patch had to take care of moving the item list CSS to Classy and make a few edits in that file, while the new patch only needs to make those edits in the file that has already been moved to Classy in HEAD.
So as far as code goes, this is good to go. We could redo the manual testing, but I'm comfortable with moving this to RTBC since the code is the same line by line.
Comment #62
alexpott@davidhernandez thanks for the screenshots. Committed 0e6abff and pushed to 8.0.x. Thanks!