Follow-up to #2372581: Rename 'page' render element to 'body', page.html.twig to body.html.twig

Problem/Motivation

The 'scripts_bottom' variable conditionally is populated in \Drupal\Core\Render\HtmlResponseAttachmentsProcessor::processAssetLibraries() and used in every html.html.twig template without any documentation.

Proposed resolution

document

Remaining tasks

review, commit.

User interface changes

None.

API changes

no

Beta phase evaluation

-

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because documentation missing
Issue priority Normal because changes only docs
Unfrozen changes Unfrozen because it only changes documentation of template variable
Disruption No
CommentFileSizeAuthor
#34 interdiff-30-34.txt1.78 KBsdstyles
#34 2542392-34.patch1.82 KBsdstyles
#30 2542392-30.patch836 bytesprabhurajn654
#25 2542392-25.patch1.84 KBgtrost
#25 interdiff-2542392-21-25.txt1.89 KBgtrost
#21 interdiff-2542392-19-21.txt732 bytesgtrost
#21 2542392-21.patch1.88 KBgtrost
#19 2542392-19.patch1.88 KBgtrost
#19 interdiff-2542392-17-19.txt1.8 KBgtrost
#17 2542392-scripts_bottom-16.patch1.62 KBandypost
#17 interdiff.txt1.16 KBandypost
#7 2542392-scripts_bottom-7.patch1.62 KBandypost
#7 interdiff.txt1.17 KBandypost
#5 2542392-scripts_bottom-5.patch1.62 KBandypost
#3 2542392-scripts_bottom-3.patch1.83 KBandypost
#3 interdiff.txt1.83 KBandypost
html-scripts-bottom.patch1.28 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Issue tags: +Quickfix
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Thanks for the issue and patch!

I think it needs a bit of work though...

+++ b/core/modules/system/templates/html.html.twig
@@ -22,6 +22,8 @@
+ * - scripts_bottom: Script tags necessary to load the JavaScript files before
+ *   clising BODY tag.

typo: clising -> closing

Also I think you want a "the" in here somewhere, and... take out one?

So this should be:

Script tags necessary to load JavaScript files before the closing BODY tag.

But really maybe BODY should not be all caps because it isn't HEAD in the rest of the docs here?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.83 KB
2542392-scripts_bottom-3.patch1.83 KB

Thanx, fixed. "HEAD" uses this caps in "head" element description
Patch also fixes "the"

Status: Needs review » Needs work

The last submitted patch, 3: 2542392-scripts_bottom-3.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs work » Needs review
FileSize
2542392-scripts_bottom-5.patch1.62 KB

fixed patch

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work 
+++ b/core/modules/system/templates/html.html.twig
@@ -20,8 +20,10 @@
+ *   closing BODY element.

So technically an element is <tag>stuff</tag>.

So there is no closing body *element*, there is a closing body *tag*.

Right?

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.17 KB
2542392-scripts_bottom-7.patch1.62 KB

sure, sorry, there's really difference (suppose element as a whole, tag as a part of template)

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Looks good to me now, thanks!

Regarding tags vs. elements...
https://en.wikipedia.org/wiki/HTML_element
http://www.456bereastreet.com/archive/200508/html_tags_vs_elements_vs_at...

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented 
+++ b/core/modules/system/templates/html.html.twig
@@ -20,8 +20,10 @@
+ *   closing BODY tag.

+++ b/core/themes/classy/templates/layout/html.html.twig
@@ -20,8 +20,10 @@
+ *   closing BODY tag.

s/BODY/<body>/

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Sorry, putting HTML inside of PHP docs is problematic. I think this should stay as it is.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented

The primary reason for me to use caps is easy to grep in core

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Needs review

Sounds like this still needs discussion.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » theme system

It sounds like the theming team should figure out how to refer to HTML tags in template docs, and decide how this should be done.

Putting the HTML tag like <body> directly into the PHP docs is not an option. It could be embedded in @code ... @endcode but that is normally for code blocks.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Assigned: Unassigned » star-szr

regex find \* .*< in *.html.twig files for reference. It seems I'd be in favour of putting in <body>

Pingging Cottser to help here.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

Looking through core, I don't see a lot of consistency in how this is treated, in comments at least. I see cases of <tag> and TAG and tag. This isn't just with html tags, but also with how protocols are written, for example. But for tags it seems to most often be <tag> in comments.

Looking for some template examples, we seem to have <tag> already in the templates.

https://api.drupal.org/api/drupal/core!modules!system!templates!table.ht...

I assume the api site escapes them. Where does this cause a problem?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Hah! Maybe the API module does escape them. It looks like it... so let's be consistent.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
FileSize
interdiff.txt1.16 KB
2542392-scripts_bottom-16.patch1.62 KB

Filed follow-up to discuss #2546248: Use consistent style to mention HTML tags in code comments

Here's a another way

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented 
+++ b/core/modules/system/templates/html.html.twig
@@ -20,8 +20,10 @@
+ * - scripts: Script tags necessary to load JavaScript files and settings
  *   in the head.

Wouldn't it be better than to use <head> here, or if not, then "in head" instead of "in the head"?

Also, with the reworking, "in" can now move to the previous line.

gtrost’s picture

gtrost CreditAttribution: gtrost at Skilld commented
FileSize
interdiff-2542392-17-19.txt1.8 KB
2542392-19.patch1.88 KB

Fixed patch using <head> and moving "in" to previous line.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs review » Needs work 
+++ b/core/themes/classy/templates/layout/html.html.twig
@@ -19,9 +19,9 @@
+ * - scripts: Script tags necessary to load JavaScript files and settings in ¶

trailing whitespace

gtrost’s picture

gtrost CreditAttribution: gtrost at Skilld commented
FileSize
2542392-21.patch1.88 KB
interdiff-2542392-19-21.txt732 bytes

Fixed trailing white space.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost at Skilld commented
Status: Needs work » Needs review

Looks good now

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Thank you for the doc clean up.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers as a volunteer commented

This looks like an excellent improvement :)

However, shouldn't we then be completely consistent?

+++ b/core/modules/system/templates/html.html.twig
@@ -19,9 +19,11 @@
+ * - styles: Style tags necessary to import all necessary CSS files in <head>.
+ * - scripts: Script tags necessary to load JavaScript files and settings in
...
+ * - scripts_bottom: Script tags necessary to load JavaScript files before the

s/Style tags/HTML/
s/Script tags/HTML/

(I propose not to say <style>, because most of the time we actually use <link rel="stylesheet">. That's unnecessarily detailed information for these docs. Hence just saying "HTML" is sufficient AFAICT.)

gtrost’s picture

gtrost CreditAttribution: gtrost at Skilld commented
FileSize
interdiff-2542392-21-25.txt1.89 KB
2542392-25.patch1.84 KB

Updated strings.

gtrost’s picture

gtrost CreditAttribution: gtrost at Skilld commented
Status: Reviewed & tested by the community » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

Thanks @Wim Leers and @gtrost. One small nit to finish this, I think:

  1. +++ b/core/modules/system/templates/html.html.twig
@@ -19,9 +19,11 @@
+ * - scripts: HTML necessary to load JavaScript files and settings in
+ *   <head>.

+++ b/core/themes/classy/templates/layout/html.html.twig
@@ -19,9 +19,11 @@
+ * - scripts: HTML necessary to load JavaScript files and settings in
+ *   <head>.

    This doesn't need to wrap to the next line anymore. 

  2. +++ b/core/modules/system/templates/html.html.twig
@@ -19,9 +19,11 @@
+ * - scripts_bottom: HTML necessary to load JavaScript files before the
+ *   closing <body> tag.

    This doesn't need to change, widows like company.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Assigned: star-szr » Unassigned

Sorry, been otherwise occupied. I can't review all the discussion right now but around where @joelpittet assigned me I agree with those things - <body> is better than BODY to me as well :)

prabhurajn654’s picture

prabhurajn654 CreditAttribution: prabhurajn654 commented
Assigned: Unassigned » prabhurajn654
prabhurajn654’s picture

prabhurajn654 CreditAttribution: prabhurajn654 commented
FileSize
2542392-30.patch836 bytes

@joelpittet I have made the changes as suggested in #27. please verify.

2542392-30.patch

prabhurajn654’s picture

prabhurajn654 CreditAttribution: prabhurajn654 commented
Assigned: prabhurajn654 » Unassigned
prabhurajn654’s picture

prabhurajn654 CreditAttribution: prabhurajn654 commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Needs work

You are missing the second template. Also please provide an interdiff with your patch. If you haven't made one before heres the docs: @see https://www.drupal.org/documentation/git/interdiff

Also it looks like the wording changed for the styles, it should have been only a line break change between #25 and yours. One line break removal for each template.

sdstyles’s picture

sdstyles CreditAttribution: sdstyles at FFW commented
Status: Needs work » Needs review
FileSize
2542392-34.patch1.82 KB
interdiff-30-34.txt1.78 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 5fcb2db and pushed to 8.0.x. Thanks!

  • alexpott committed 5fcb2db on 8.0.x 
    Issue #2542392 by andypost, gtrost, sdstyles, prabhurajn654, jhodgdon,...

Status: Fixed » Closed (fixed)

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

prabhurajn654’s picture

prabhurajn654 CreditAttribution: prabhurajn654 as a volunteer and at Srijan | A Material+ Company commented