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.

Alter wight of comment field

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.

Comment field appearing last

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.
2455211 UI Changes

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.
Files: 
CommentFileSizeAuthor
#87 comment_field_displayed-2455211-87.patch4.4 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,717 pass(es). View

Comments

nlisgo’s picture

Issue summary: View changes
nlisgo’s picture

Component: comment.module » Bartik theme

I 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.

nlisgo’s picture

Status: Active » Needs review
FileSize
580 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,231 pass(es). View

This should do it.

nlisgo’s picture

There 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.

nlisgo’s picture

Issue summary: View changes
nlisgo’s picture

The 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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +frontend, +markup
Related issues: +#2428509: Comment links weight should be managed by view display

We have no tests for bartik and since comment a field its position should be controlled by entity display

alexpott’s picture

Assigned: Unassigned » emma.maria
Status: Reviewed & tested by the community » Needs review

Assigning to emma.maria to have a look at the impacts to bartik markup

emma.maria’s picture

Assigned: emma.maria » Unassigned
Status: Needs review » Needs work
Issue tags: +CSS

node.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 :

  1. Change the default order of fields on install within Content Display, so Links automatically print above Comments - found within core.entity_view_display.node.article.default.yml
  2. Remove the {{ content.links }} declaration within the template, and just have the node template call all {{ content }} where it currently excludes links.
  3. Find the best way to add .node__link class back to the links field.

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.

nlisgo’s picture

@emma.maria thanks for the feedback. This gives me something to work on en route to Montpellier tomorrow. :)

nlisgo’s picture

Status: Needs work » Needs review
FileSize
3.29 KB
3.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,741 pass(es). View

I believe I have addressed the feedback in #9. Please review.

nlisgo’s picture

FileSize
488 bytes
3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,569 pass(es). View

I forgot to check if there were any links before outputting the markup.

emma.maria’s picture

From 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.

emma.maria’s picture

Issue tags: +Twig
davidhernandez’s picture

The - 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?

nlisgo’s picture

@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?

davidhernandez’s picture

I'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.

nlisgo’s picture

FileSize
49.15 KB

I 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.

davidhernandez’s picture

Joel 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.

emma.maria’s picture

I 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.

nlisgo’s picture

FileSize
447 bytes
361.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,438 pass(es). View
447 bytes

I'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.

nlisgo’s picture

Issue tags: +drupaldevdays
joelpittet’s picture

Status: Needs review » Needs work

@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

nlisgo’s picture

Status: Needs work » Needs review
FileSize
447 bytes
3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,440 pass(es). View

Here is the patch again

davidhernandez’s picture

I 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.

davidhernandez’s picture

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Novice
FileSize
98.54 KB
46.72 KB
794.99 KB

The 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...

.node__content {
  font-size: 1.071em;
}

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.

haasontwerp’s picture

Assigned: Unassigned » haasontwerp

Going to fix the font size.

haasontwerp’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,385 pass(es). View
224.02 KB
29.87 KB
2.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View

Added new font-sizes to comments and links, restoring them to the same display as before the .node-content declaration.

haasontwerp’s picture

haasontwerp’s picture

FileSize
898 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,382 pass(es). View

Clean patch with only css changes.

haasontwerp’s picture

Assigned: haasontwerp » Unassigned
haasontwerp’s picture

FileSize
4.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,433 pass(es). View
898 bytes

This latest patch contains the combined work from #24 and #30, and i've also supplied an interdiff.

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
665.65 KB
205.67 KB

The 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.
 

 

+++ b/core/themes/bartik/css/components/comments.css
@@ -7,6 +7,9 @@
+.field-node--comment {
+  font-size: 0.931em;
+}
haasontwerp’s picture

Apparently 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.

haasontwerp’s picture

haasontwerp’s picture

Status: Needs work » Needs review
haasontwerp’s picture

FileSize
149 KB
4.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch comment_field_displayed-2455211-38.patch. Unable to apply patch. See the log in the details link for more information. View

Ok 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.

emma.maria’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
728.62 KB
500.05 KB

Yay! 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.

haasontwerp’s picture

Issue summary: View changes
haasontwerp’s picture

Issue summary: View changes
emma.maria’s picture

Issue summary: View changes

Suggested commit message

git commit -m 'Issue #2455211 by haasontwerp, nlisgo, emma.maria, davidhernandez: Comment field displayed last regardless of assigned weight'

emma.maria’s picture

Issue summary: View changes

Added beta evaluation

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: comment_field_displayed-2455211-38.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: comment_field_displayed-2455211-38.patch, failed testing.

andypost’s picture

Issue tags: +Needs reroll
nlisgo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,275 pass(es). View
andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone, great work! The screenshots and clear reviews are very helpful. I added @emma.maria and @davidhernandez to the credit field per #42.

+++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml
@@ -39,15 +39,15 @@ content:
+  links:
+    weight: 100
   comment:
     label: above
     type: comment_default
-    weight: 20
+    weight: 110
     settings:
       pager_id: 0
     third_party_settings: {  }
-  links:
-    weight: 100
 hidden:
   langcode: true

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.

xjm’s picture

Forgot the tag. :)

nlisgo’s picture

Issue summary: View changes
Issue tags: -Novice, -Needs issue summary update

The 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.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
Related issues: +#2031883: Markup for: comment.html.twig

Looks all points are addressed, back to RTBC
Once this in #2031883: Markup for: comment.html.twig will need re-roll

xjm’s picture

Status: Reviewed & tested by the community » Needs review

No, 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!

nlisgo’s picture

Issue tags: +Needs screenshots
andypost’s picture

We 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

andypost’s picture

Also I'm sure that classy should be cleaned-up too

nlisgo’s picture

@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?

nlisgo’s picture

And @andypost is right there is a small amount of work to do in the classy theme also.

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
145.16 KB
145.09 KB
140.18 KB
138.69 KB
86.48 KB
86.48 KB

From Screenshots, seems like everything is fine, no regression issues. Please find the screenshots below.

alt

alt

alt

alt

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.

alt

alt

Manjit.Singh’s picture

Issue tags: -Needs screenshots

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: comment_field_displayed-2455211-49.patch, failed testing.

emma.maria’s picture

Issue tags: +Needs screenshots

We need screenshots for what happens with Classy.

Manjit.Singh’s picture

@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

alt

[Before] Without changing weight

alt

[After] move at top by changing weight to -2

alt

[After] Without changing weight

alt

Manjit.Singh’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
andypost’s picture

Looks classy works fine, so we just need to check all node templates that could be changed since last patch

HOG’s picture

FileSize
4.06 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,266 pass(es). View
4.19 KB

Minimize noise in the config file

andypost’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
702 bytes

I think it's now ready, Updated issue summary about weight of comment field.

Interdiff is wrong, so attached actual one

+++ b/core/profiles/standard/config/install/core.entity_view_display.node.article.default.yml
@@ -42,7 +42,7 @@ content:
   comment:
     label: above
     type: comment_default
-    weight: 20
+    weight: 110

this is change of default config - not sure we need hook_update_N() for that because this could be fixed via UI now

lauriii’s picture

Looks good, RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This 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.

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh

working on change records !!

Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs change record

I have created a CR, Can someone Please review it. :) https://www.drupal.org/node/2532384

cilefen’s picture

The 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.

Description:
1. Change the default order of fields on install within Content Display, so Links automatically print above Comments
core.entity_view_display.node.article.default.yml
2. Call {{ content }} instead of {{ content.links }}
3. Find the best way to add .node__link class back to the links field.

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.

andypost’s picture

Issue summary: View changes
FileSize
831 bytes
4.62 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,455 pass(es). View
15.12 KB
13.99 KB
23.75 KB
32.06 KB
13.33 KB
18.04 KB

New 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

lauriii’s picture

Status: Needs review » Needs work

I 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:

+++ b/core/themes/classy/templates/content/links--node.html.twig
@@ -0,0 +1,43 @@
+ * Bartik's theme implementation to display the node links.

The comment is still wrong. This is now Classy's theme implementation

Manjit.Singh’s picture

@andypost Thanks for the CR suggestions, I have updated it. Please verify.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
4.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2455211-comment_field_display-80.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 80: 2455211-comment_field_display-80.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
4.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2455211-comment_field_display-83.patch. Unable to apply patch. See the log in the details link for more information. View

re-roll

HOG’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
96.65 KB
146.17 KB

comments in article
comments admin

Tested new patch.

emma.maria’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
FileSize
92.26 KB

The patch in #83 no longer applies because of #2398463: Clean up the "content" component in Bartik, it needs a reroll.

The last submitted patch, 83: 2455211-comment_field_display-83.patch, failed testing.

nlisgo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.4 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,717 pass(es). View
andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc, let's get this finally in

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: comment_field_displayed-2455211-87.patch, failed testing.

Status: Needs work » Needs review
rudraram’s picture

Many submitted patches are getting failed in the first test. Submitted for re-test.

rudraram’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 87: comment_field_displayed-2455211-87.patch, failed testing.

Status: Needs work » Needs review
emma.maria’s picture

Status: Needs review » Reviewed & tested by the community

Setting back to RTBC again :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

  • alexpott committed e2f5705 on 8.0.x
    Issue #2455211 by nlisgo, haasontwerp, andypost, HOG, emma.maria, Manjit...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Status: Closed (fixed) » Needs work

The last submitted patch, 87: comment_field_displayed-2455211-87.patch, failed testing.

andypost’s picture

Status: Needs work » Closed (fixed)