Suggested commit message
git commit -m 'Issue #2455211 by haasontwerp, nlisgo, emma.maria, davidhernandez, HOG: Comment field displayed last regardless of assigned weight'
Problem/Motivation
This only seems to be an issue with the Bartik theme.
Login as admin and visit /admin/structure/types/manage/article/display to manage the display of the Article content type.
Move the comment field up to the top and save.
Add an article node and visit the node page you have just created.
The comment field should appear at the top but it appears at the bottom of the page.
In my experience this only appears to be an issue with the default comment field. If you add a new comment field and assign it a weight it will appear as expected.
Proposed resolution
Make the comment field respect the weight in the template, so it can be placed anywhere on node and it follows the same placement logic as the other fields.
Update default article display config for comment field to follow it after node links.
Remaining tasks
commit.
User interface changes
Changing a weight of comment field actually changes order.
API changes
None.
Beta phase evaluation
Issue category | Bug because Bartik is currently hardcoding the order of node fields and going against allowed Drupal core behaviour. |
---|---|
Issue priority | Normal because functionality is not broken it's just forcefully restricted. |
Unfrozen changes | Much of the patch can be considered to be unfrozen because it only changes CSS and markup. But the adjustment of default comment field weight in core.entity_view_display.node.article.default.yml (to make it display right after node links) means we must consider whether this is a prioritized change. |
Prioritized changes | The main goal of this issue is fixing a restriction introduced by forcing comments and links to be output in a position which may differ from the weights in the 'Manage Display' admin UI. Adjusting the weights of some fields (comments and links) in the 'Manage Display' section is not working for users of the Bartik theme which is the default theme. |
Disruption | Minor disruption as the links field will appear above the comments field for all of the themes but at least that will match the weights in the 'Manage Display' admin UI. |
Comment | File | Size | Author |
---|---|---|---|
#87 | comment_field_displayed-2455211-87.patch | 4.4 KB | nlisgo |
Comments
Comment #1
nlisgo CreditAttribution: nlisgo commentedComment #2
nlisgo CreditAttribution: nlisgo commentedI have since learned that this is an issue with the Bartik theme. If you switch to using one of the other available themes it is no longer an issue.
Comment #3
nlisgo CreditAttribution: nlisgo commentedThis should do it.
Comment #4
nlisgo CreditAttribution: nlisgo commentedThere has been an attempt to force the comments field to be displayed below the rest of the fields regardless of its' position in the Manage Display UI but it only seems to be present in the Bartik theme and I would think it would confuse most users, except those who implemented the adjustment to the node template, that it behaves in this way.
Comment #5
nlisgo CreditAttribution: nlisgo commentedComment #6
nlisgo CreditAttribution: nlisgo commentedThe decision to enforce the display of the default comments field last was made as part of #2004252: node.html.twig template
I will try to reach out to one of the contributors to that issue to see how they feel about it.
Comment #7
andypostWe have no tests for bartik and since comment a field its position should be controlled by entity display
Comment #8
alexpottAssigning to emma.maria to have a look at the impacts to bartik markup
Comment #9
emma.marianode.html.twig currently prints the comments and links separate to the rest of the content to control the order of the fields and add needed markup for styling.
By default in Bartik we want the Links to print above Comments.
Currently:
- Links are taken out of the main content section and printed below in their own section with their own class for styling.
- Comments are then forced to print below the links.
I agree that if the fields are available in configuration to be moved around, we should let them do that. A default theme for Drupal shouldn't impose these limitations.
I also do not have a problem changing the structure of the node template. We do not need to separate links into a .node__links section. Links are available as a field in the node content so they can print within the content section.
However taking the current behaviour out of the node template means Links loses a class that it needs to align them to the right. So we possibly need to add a field template to add this class to this field.
Work to do :
This should result in the content printing in the same that it currently does, with the added bonus of no restrictions on which order a user can display the fields.
Comment #10
nlisgo CreditAttribution: nlisgo commented@emma.maria thanks for the feedback. This gives me something to work on en route to Montpellier tomorrow. :)
Comment #11
nlisgo CreditAttribution: nlisgo commentedI believe I have addressed the feedback in #9. Please review.
Comment #12
nlisgo CreditAttribution: nlisgo commentedI forgot to check if there were any links before outputting the markup.
Comment #13
emma.mariaFrom what I asked in #9 the patch in #12 achieves the following:
1. The content display for articles in Bartik now has Links followed by Comments in the content order.
2. The node template in Bartik now just prints the node content in one section and does not hardcode the position of any of the content fields.
Work still needed.
I need a Twig person to code review links--node.html.twig.
We have added this template to avoid hard coding a field's placement and markup in the node.html.twig template, we need a template in Bartik to theme the Links in nodes. Bartik shows links on the right hand side so we need the .node__links class.
Comment #14
emma.mariaComment #15
davidhernandezThe
-
in the if is for removing whitespace. I'm not sure it really matters in this case. Potentially, if links are rendered inline within something this could cause a space issue, but I don't think so. I wouldn't be surprised if it is in the original template solely to make the markup consistent for tests.I don't, though, understand the reason for the if; here and in the original template. What is the scenario where there are no links but the template is being processed?
Comment #16
nlisgo CreditAttribution: nlisgo commented@davidhernandez perhaps you're right and we don't need to check for links. This was just in place in the existing template. Would it be appropriate to raise a separate issue for that or do you think we should address it here?
Comment #17
davidhernandezI'm checking with joelpittet about the if and whitespace modifiers, since he created teh original template, just for my own satiation. I want to make sure my understanding is correct before saying remove it. I definitely would not address anything related to the main links.html.twig template here. That is unrelated.
Comment #18
nlisgo CreditAttribution: nlisgo commentedI just tested the effect of removing the if condition in the core/themes/bartik/templates/links--node.html.twig and the div appears even if there is no links provided. So, for now, I believe we need it in there. Maybe there is a separate issue we need to raise if that is not the expected or desired behaviour.
Comment #19
davidhernandezJoel confirmed that the whitespace modifiers are likely unnecessary, and upload a patch to verify that they aren't needed for tests. That patch passed. So if someone gets bored we might remove them from the main links file. (in another issue)
The if though I'm still iffy about. :) I did all my testing with the original template, and did not find any issue removing it.
Comment #20
emma.mariaI am happy with removing the whitespace thing and keeping the if statement. Without the if statement an empty div prints when there are no links which is an often occurrence in nodes, as shown in #18.
Comment #21
nlisgo CreditAttribution: nlisgo commentedI've removed the whitespace removal from the core/themes/bartik/templates/links--node.html.twig file.
I have left in the if statement as without it an empty
<div class="node__links"></div>
will be output even if there are no links.Comment #22
nlisgo CreditAttribution: nlisgo commentedComment #23
joelpittet@nlisgo something went a bit sideways on that last patch it's 361K in size and should be much smaller maybe 1K?
Likely forgot to rebase is my guess?
Also if someone want's to take credit for it, here's the patch for #19 : https://www.drupal.org/node/2465399#comment-9829099
Comment #24
nlisgo CreditAttribution: nlisgo commentedHere is the patch again
Comment #25
davidhernandezI talked to Joel a bit on IRC and agreed that this if-business is a bit weird and there might be some legacy reason why, but that is not this issues burden. Leave it.
Comment #26
davidhernandezAdded an issue. #2472591: Remove whitespace modifiers from links.html.twig
Comment #27
emma.mariaThe markup prints out as we want it to. All fields in a node now print under one wrapper in the order you set them to.
The order of the fields by default for the content type Article is now correct.
The new Twig template that was created for this issue has been looked at and approved in #19 and #25.
However one issue I noticed when I did some visual regression testing. Because of current styling in Bartik any object added inside the .node__content div has it's font size increased due to a font size being enforced on this div wrapper....
The code that causes this...
The above code should not exist in Bartik as every element within .node__content declares its own font size also.
We will be tackling legacy problems like this within this issue... #2399247: Reduce number of different font-sizes in Bartik for better consistency.
So for now as Bartik already has a billion font size declarations and it needs a huge overhaul, to have this patch ready we need to add font-size declarations to the .node__link and .field-node--comment wrappers to counteract the 1.071em size being applied to them and get them to their original sizes.
Setting to Needs work and also tagging with Novice as I have fully defined the work that needs to be carried out.
Comment #28
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedGoing to fix the font size.
Comment #29
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedAdded new font-sizes to comments and links, restoring them to the same display as before the .node-content declaration.
Comment #30
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #31
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedClean patch with only css changes.
Comment #32
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #33
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedThis latest patch contains the combined work from #24 and #30, and i've also supplied an interdiff.
Comment #34
emma.mariaThe patch is so close to being finished!
The links font size is now fine, but comments still need work. See below...
The computed pixel size of the comment section at HEAD is 14px. So we need to get the below value to calculate as close as we can to 14px to keep them the same size.
Comment #35
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedApparently field-comments and node__links are no longer in .node__content (see attached image), so the .node__content { font-size: 1.071em; } has no effect on those fields anymore. #24 can be applied without visual changes to comments or node links.
Comment #36
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #37
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #38
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedOk forget everything i said, i had to cache clear to see the html changes. I altered the comments field font-size, now it's an exact match to the way it was before.
Comment #39
emma.mariaYay! Reviewing the patch in #38, all of the font sizes now match up with the sizes before the patch.
I have also attached a file showing all of the screen widths for the before and after patch visual testing.
Setting this to RTBC.
Comment #40
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #41
haasontwerp CreditAttribution: haasontwerp at Haas Ontwerp commentedComment #42
emma.mariaSuggested commit message
git commit -m 'Issue #2455211 by haasontwerp, nlisgo, emma.maria, davidhernandez: Comment field displayed last regardless of assigned weight'
Comment #43
emma.mariaAdded beta evaluation
Comment #45
joelpittetUnrelated test failure, back to RTBC
Comment #48
andypostComment #49
nlisgo CreditAttribution: nlisgo commentedComment #50
andypostback to rtbc
Comment #51
xjmThanks everyone, great work! The screenshots and clear reviews are very helpful. I added @emma.maria and @davidhernandez to the credit field per #42.
This is not CSS or markup, so the beta evaluation is not 100% complete here. Could we document the reason for this weight change in the summary, or if it's not needed, remove it from the patch? Also, note that this change would not take effect on existing sites, only on new ones, so we should consider how those sites would look without this change but only the improved CSS and templates. Will they still display correctly in Bartik?
Also, changing the weights for the default article display would presumably change more than Bartik. Has anyone tested other themes? If so let's get them documented with screenshots here too.
Comment #52
xjmForgot the tag. :)
Comment #53
nlisgo CreditAttribution: nlisgo commentedThe weight change in core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml was requested in #9.
Changing the weights of the links in the default display will change more than just Bartik. The Bartik theme is the only theme where the comments field display is fixed in the node template.
I, personally, do not feel strongly about the position of the links and the comments. I do feel though that the position of these fields should be determined by the weights in the 'Manage Display' UI rather than fixed in the node template for core themes.
I've adjusted the beta evaluation.
Comment #54
andypostLooks all points are addressed, back to RTBC
Once this in #2031883: Markup for: comment.html.twig will need re-roll
Comment #55
xjmNo, all feedback has not been addressed. Please read #51. We need to check the impact on other themes, and also verify that how Bartik looks is still okay when the new weights are ignored (since existing sites will not receive this change and since users can change it anyway). Thanks!
Comment #56
nlisgo CreditAttribution: nlisgo commentedComment #57
andypostWe need to do this change in one issue, so probably #2428509: Comment links weight should be managed by view display could be closed and merged to do the change once with one change record
Comment #58
andypostAlso I'm sure that classy should be cleaned-up too
Comment #59
nlisgo CreditAttribution: nlisgo commented@andypost, I see the wisdom in combining the issues - if I am understanding you correctly. The issue that connects both this task and #2428509: Comment links weight should be managed by view display is that we should expect that the weight adjustments in 'Manage Display' should correspond to the order in which fields are displayed for the core themes.
@emma.maria what do you think?
Comment #60
nlisgo CreditAttribution: nlisgo commentedAnd @andypost is right there is a small amount of work to do in the classy theme also.
Comment #61
Manjit.SinghFrom Screenshots, seems like everything is fine, no regression issues. Please find the screenshots below.
Everything is fine in Seven theme.. When we are changing weight of comments from manage display, It reflects on frontend easily. Facing no issues.
Please check before/after screenshots of seven theme.
Comment #62
Manjit.SinghComment #65
emma.mariaWe need screenshots for what happens with Classy.
Comment #67
Manjit.Singh@emma.maria I have tested in classy as well. There are some screenshots with changing weight and without changing weight in /admin/structure/types/manage/article/display
[Before] move at top by changing weight to -2
[Before] Without changing weight
[After] move at top by changing weight to -2
[After] Without changing weight
Comment #68
Manjit.SinghComment #69
andypostLooks classy works fine, so we just need to check all node templates that could be changed since last patch
Comment #70
HOG CreditAttribution: HOG at Skilld commentedMinimize noise in the config file
Comment #71
andypostI think it's now ready, Updated issue summary about weight of comment field.
Interdiff is wrong, so attached actual one
this is change of default config - not sure we need
hook_update_N()
for that because this could be fixed via UI nowComment #72
lauriiiLooks good, RTBC++
Comment #73
alexpottThis is a nice change - having fields that are movable in the UI but not actually in the theme is very frustrating for users. I've considered the upgrade path for this change. On sites that are using Bartik as their theme and have comments enabled this will result in a visual change because the comment field weight in the live site will not be changed by this patch. However the fix is simple just re-order the fields in the UI. Given that Drupal 8 is in beta and sites are not broken by this patch, I think it is okay to just publish a CR for site builders which tells people to use the UI to fix this.
Comment #74
Manjit.Singhworking on change records !!
Comment #75
Manjit.SinghI have created a CR, Can someone Please review it. :) https://www.drupal.org/node/2532384
Comment #76
cilefen CreditAttribution: cilefen commentedThe CR title is "Changing comment field weight to print links above comments" which doesn't really match what they usually look like. I would expect "Printing links above comments requires ... (something, what?)".
Also the body of the CR doesn't make sense to me.
We need an actual description explaining what this "thing" is about. 1, 2, and 3 are the steps to do it or something? It is unclear.
Comment #77
andypostNew patch, classy theme should be updated too.
Once we do change record then better to have the same way of rendering of content links all over core, so we have the same issue with comments #2428509: Comment links weight should be managed by view display
Bartik is inherited from classy so new template should live in that, CSS for them only needed in bartik
Field displays for content (links above tags)
links--node template was patched to display "links test:" before links to check template inheritance
Classy theme
Bartik
CR
: basically "Content links position managed by node display" describes better
1) comment field weight increased, check your displays
2) ++ you still can use {{ content.links }} but with cons... for UI
3) node links - it's all about bartik, no node/comment code affected
Comment #78
lauriiiI made the CR a little bit more explicit but overall it looked fine. Patch looks fine but one minor point still needs to be fixed:
The comment is still wrong. This is now Classy's theme implementation
Comment #79
Manjit.Singh@andypost Thanks for the CR suggestions, I have updated it. Please verify.
Comment #80
andypostFixed comment
Also "html" no more an option for links https://www.drupal.org/node/2392803 so filed #2535586: Clean-up "links" templates from removed "html" option
Comment #83
andypostre-roll
Comment #84
HOG CreditAttribution: HOG at Skilld commentedTested new patch.
Comment #85
emma.mariaThe patch in #83 no longer applies because of #2398463: Clean up the "content" component in Bartik, it needs a reroll.
Comment #87
nlisgo CreditAttribution: nlisgo commentedComment #88
andypostback to rtbc, let's get this finally in
Comment #91
rudraram CreditAttribution: rudraram at Axelerant commentedMany submitted patches are getting failed in the first test. Submitted for re-test.
Comment #92
rudraram CreditAttribution: rudraram at Axelerant commentedComment #93
andypostback to rtbc
Comment #96
emma.mariaSetting back to RTBC again :)
Comment #97
alexpottCommitted e2f5705 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary. I agree that this is a bug - if themes do this they break the field UI.
Comment #101
andypost