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
Proposed resolution
Add comment links via entity display element
Remaining tasks
Fix template, add tests
User interface changes
comment links are ordered according weight in view display
API changes
no
Comment | File | Size | Author |
---|---|---|---|
#40 | comment-display.png | 54.53 KB | emma.maria |
#40 | comment.png | 73.43 KB | emma.maria |
#39 | 2428509-comment_links_weight-39.patch | 2.76 KB | andypost |
Comments
Comment #1
fotuzlab CreditAttribution: fotuzlab commentedComment #2
rahul.shindeComment #3
rahul.shindeComment #4
rahul.shindeComment #5
andypostGreat! now we need to add tests
Comment #6
rahul.shindeComment #7
rahul.shindeUploading new patch with web-test and few changes.
Comment #8
rahul.shindeComment #9
rahul.shindeWe need to call node page to check functionality. Input from @andypost
Same goes here too.
Comment #10
rahul.shindeAdding two patches,
Comment #12
andypostThere's test and fix
Comment #13
Wim LeersThe "Links" component is also present in HEAD, but doesn't have any effect because the templates override this. So +1 to this!
Doesn't this mean that now links won't be using
<nav>
?Comment #14
alexpottWhat about this
<nav>
tags?So links will now always come above the signature?
Comment #15
rahul.shinde@alexpott,
For 1, Can we write things in Preprocessor, which is obviously not so cool thing to do.
For 2. There is a ticket saying remove signature and make it as contrib, see #1548204: Remove user signature and move it to contrib
Comment #16
andypostAlso we can use
#prefix/suffix
or#theme_wrapper => tag
Comment #17
andypostanother option to use own
field__comment__links
template, but that needs profilingComment #18
andypostNAV added only by bartik, so move it to preprocess
PS: theme wrappers does not work for "html_tag" because it's a render element not a theme function, #17 will not work because link is not a field
Comment #19
andypostComment #20
emma.mariaThis issues addresses the default themes so I'm adding the frontend tag for more exposure.
Comment #21
andypostWill require re-roll after #2031883: Markup for: comment.html.twig
Comment #22
star-szrWhat happens with this if there are no links?
Would be nice to s/NAV/
<nav>
/ I think :)Comment #25
andypostre-roll and fixed comment
@Cottser I'm sure that HTML-tags are always written in caps, see #2542392-8: Document scripts_bottom template variable
maybe better check with
Element::children()
?Comment #26
andypostI'm pretty sure that "nav" here is wring, because comment links are not a part of main navigation
Comment #29
emma.mariaThanks @andypost for pointing this out.
Yes nav should be used for major navigation and here it's being applied to an action list for each comment. I am happy to remove it I agree it's overkill, especially if we have to also add a preprocess to Bartik to support it.
For CSS purposes the
<nav>
wrapper is only serving the purpose of providing a 1px padding which was added incorrectly in #2031883: Markup for: comment.html.twig. I am happy to remove the nav wrapper and convert the padding to a line height on the links list.I can add the fix for the v slight visual issue removing the nav causes using the patch in #26.
Comment #30
andypostFixed 1px!
@emma.maria awesome find!
Comment #32
andypostsomehow no comment body rendered...
Comment #34
andypostFixed tests, looks iterate through elements is only way
Comment #35
larowlanif you re-roll, and while we're touching this - can re remove the reliance on random here (there's a meta/initiative to remove uses of random in tests to prevent random fails)
Other than that +1 RTBC - but leaving for Emma as this touches Bartik too - assigning accordingly
Comment #36
andypostI'm pretty sure that this one should use
randomString()
to generate comment body because comment body contains user input that is not "machine name"Comment #37
andypostis it possible to get this in before release?
Comment #38
Truptti CreditAttribution: Truptti at Axelerant commentedGot error on applying patch in #34,attaching snapshot for reference.
Comment #39
andypostre-roll
Comment #40
emma.mariaArgh sorry for holding this up! I'm happy with everything Bartik in this issue.
Also for good measure, I tested it out and links can now print above the comment. Woop!
Comment #41
star-szrThanks everyone.
It's too late for 8.0.x for this one, bumping to 8.1.x for now. I'm also not sure what the test here is really doing, the test doesn't seem to be testing Bartik (it's testing Stark) and Bartik is the only functional (non-test) code we're changing. Am I missing something? I'm not against adding test coverage but if there's similar coverage elsewhere then I don't think we should add it on account of this issue. I ran the test locally and it passes both with and without the functional part of the patch.
Should the component be Bartik for this?
Comment #42
andypostYep, test should switch theme to Bartik
Maybe better to extract test to separate method with Bartik theme enabled?
@larowlan any ideas?
Comment #43
andypostSuppose it not too late for d8
Comment #44
alexpottGiven this was pushed back from 8.0.x to 8.1.x and forgotten about and has the relevant sign off from the sub system maintainers I'm going to commit this to 8.2.x in time for 8.2.0.
Committed and pushed 89df46f to 8.3.x and 3cd6347 to 8.2.x. Thanks!
Comment #47
andypostFiled follow-up to extend tests by #41 #2792243: Extend test coverage for comment links weight to classy theme
Comment #49
lauriiiJust moving to the right queue