Problem/Motivation
node.html.twig markup needs a cleanup after the move from phptemplate to twig.
Both Stark & Bartik needs this, but is not expected to have the same output.
Issues have been raised doing the cleanup those are created as followup issues to seperate the issues & beeing able to move forward
Proposed resolution
basic stark template:
<article class="{{ attributes.class }}"{{ attributes|without('class') }}>
{{ title_prefix }}
{% if not page %}
<h2{{ title_attributes }}>
<a href="{{ url }}" rel="bookmark">{{ label }}</a>
</h2>
{% endif %}
{{ title_suffix }}
{% if display_submitted %}
<footer>
<footer class="node__meta">
{{ author }}
{{ submitted }}
</footer>
{% endif %}
<div class="node__content"{{ content_attributes }}>
{{ content|without('links') }}
</div>
{% if content.links %}
<div class="node__links">
{{ content.links }}
</div>
{% endif %}
</article>
attributes.class:
.node
.node--$nodetype
.node--promoted
.node--sticky
.node--unpublished
.node--preview
Remaining tasks
* provide screenshots
* inline documentation
* stop score creeping
User interface changes
API changes
none
Please note, we need to keep the variables in sync with the comment template:
#2031883: Markup for: comment.html.twig, issue for global changes #2004966: Markup and variable cleanup for titles and attributes in all templates
Part of: #1980004: [meta] Creating Dream Markup
original proposal
During the code sprint at DrupalCon Portland, Jen Lampton proposed an improved version for node.html.twig:
Proposal
<article id="node--{{ nid }}" class="clearfix {{ attributes.class }}"{{ attributes }}>
{{ title.prefix }}
{% if not page %}
<h2{{ title.attributes }}><a href="{{ url }}" rel="bookmark">{{ title.label }}</a></h2>
{% endif %}
{{ title.suffix }}
{% if display_submitted %}
<footer>
{{ author|view_mode('bio') }}
{% trans %}
Submitted by {{ author.name }} on {{ published_date|format_date('medium') }}
{% endtrans %}
</footer>
{% endif %}
<div{{ content.attributes }}>
{{ content.field_x }}
{{ content.remainder }}
</div>
{{ links }}
{{ comments }}
</article>
Please note, we need to keep the variables in sync with the comment template:
#2031883: Markup for: comment.html.twig, issue for global changes #2004966: Markup and variable cleanup for titles and attributes in all templates
Comment | File | Size | Author |
---|---|---|---|
#205 | 2004252-node-205.patch | 27.38 KB | sarahjean |
#190 | Screen Shot 2014-05-07 at 00.06.42.png | 475.22 KB | Jeff Burnz |
#181 | stark-node-pre.png | 1.26 MB | mortendk |
#181 | stark-node-post.png | 1.26 MB | mortendk |
#181 | stark-frontpage-pre.png | 1.05 MB | mortendk |
Comments
Comment #1
scor CreditAttribution: scor commentedtagging
Comment #2
pixelwhip CreditAttribution: pixelwhip commentedRemove ID attribute or make it a class instead
We should remove the node-[nid] id. IDs need to be unique and I don't think we can safely assume a given node will only appear once on a given page. It's feasible that a single node may show up in multiple views on a single page or it may be shown more than once using different view modes.
If there is no specific functionality dependent on this id, we should remove it completely or move it into the class array via preprocess.
Remove clearfix class
There is no layout CSS attached to the node that I know of that would require clearfixing. If a theme needs to create a multi-column layout within a node, they can add a clearfix class then. If we do need to keep it, is there a reason we want this hardcoded in the template rather than adding it to the classes array?
Remove comment hiding comment?
{# We hide the comments and links now so that we can render them later. #}
Is that comment relevant anymore? I don't see where we are hiding comments.
Comment #3
ry5n CreditAttribution: ry5n commented#2: All good points. Some additional questions:
- Is the ‘submitted’ class the best name for this? Can we omit the class entirely?
- It looks like {{content.field_x}} and {{content.remainder}} are an example; in the actual template would this simply be {{content}}?
Comment #4
dasjoi have looked at this now and tried to change the css classes so that they match the CSS Coding Standards:
attached is a patch to
node_submitted
instead ofsubmitted
according to the css coding standards, see #3regarding to "Remove comment hiding comment" in #2:
as of the code, we are still hiding those elements.
core node.html.twig vs. bartik node.html.twig
bartik currently is using its own node template. i have looked at the difference (see the second attached file).
does this really make sense? i have the feeling, that the current implementation is more about inconsistencies rather than added value for the bartik theme.
we might want to put this discussion in a separate issue.
Comment #5
dasjoremoved the comment related to the id attribute
Comment #7
jenlamptonYes,
{{ content.field_x }}
was an example, but in the actual template it would be this:this will allow teme developers to change or remove the div tag around the content directly in the node template. Printing
{{ content }}
will include the surrounding div tag.We'll need to clean up the way comments and links are added (in node_view? in preorocess?). They should not be "inside" content in the first place, and when we get them out of there we won't need to hide anything anymore. It would be great to see that in this patch as well.
For differnces between core node.html.twig and bartik's node.html.twig:
Comment #8
dasjoThanks for the feedback!
i have updated the patch with some attempts:
i have introduced
comment_preprocess_node
as i think this is the only way to expose comments as separate variables in a sane way.the above code related to rendering the comments for a node still remains in
comment_node_view
, but maybe we want to move this over to the preprocess function?Comment #10
ry5n CreditAttribution: ry5n commented@jenlampton Thanks for clarifying with #7.
With this:
Would attributes.class output additional classes by default? I ask because
node--{{ node.nid }}
is using the “component variant” naming convention, which means we’d need a 'node' class too:class="node node--{{ node.nid }} …"
. I wonder if we even need the node id at all by default. A minimal version could be justclass="node node--{{ node.type }}"
.Comment #11
jenlamptonYes. I believe
node
andnode--[type]
as well asnode--published
and maybe evennode--sticky
are all added there by default. If there isn't anything in core that needs anode-[nid]
class added, can we just move that to Bartik?edit:
here are the classes I get from stark (but it's currently the same in bartik):
The goal here is to make core as clean as we can, and move anything base-theme-like into Barrik, as per #1854344: [meta] Goals for core themes: Make Stark as semantic as possible; Make Bartik a better prospective base theme
Additionally
{{ node.nid }}
should not be used. It should have been{{ nid }}
as per the issue summary ;)Comment #12
ry5n CreditAttribution: ry5n commented@jenlampton Sounds good then! I guess we’ll need to check if anything needs the nid in core, but if not, this can absolutely get moved to Bartik. One less default class :)
Comment #13
jenlamptonGiving it another pass. New changes include:
{{ node.nid }}
changed to{{ nid }}
and moved to Bartik (but we should confirm this doesn't break anything){{ submitted }}
{{ user_picture }}
to{{ author }}
{{ node_url }}
to{{ url }}
- in node module{{ node_url }}
to{{ url }}
- in RDF moduleI also created a new issue for the title & attributes cleanup: #2004966: Markup and variable cleanup for titles and attributes in all templates
Comment #14
ry5n CreditAttribution: ry5n commentedI searched core for the following strings and found nothing referencing the node id from the markup:
- .node-
- #node-
- getElementById('node
- getElementById("node
#13 is looking really great. Some details:
Let’s remove the
<p>
wrapper around {{ submitted }}. Bartik’s node template doesn’t have it.class="node__{{ nid }}
should beclass="node--{{ nid }}
. The double-underscore denotes a child element; the double-dash is a modifier class (special-casing the 'node' class). Because if this, I’d also like the modifier to come after the default classes:<article class="{{ attributes.class }} node--{{ nid }} clearfix"…
.Comment #14.0
ry5n CreditAttribution: ry5n commentedremainder change
Comment #14.1
jenlamptonupdated as per recent comment
Comment #15
jenlampton@ry5n thanks!
I've updated the issue summary to reflect your changes in #14, and updated the comment issue to match:
#2031883: Markup for: comment.html.twig
Comment #15.0
jenlamptonadd link to comment
Comment #15.1
jenlamptonblocked
Comment #16
steveoliver CreditAttribution: steveoliver commentedBot?
Comment #18
mortendk CreditAttribution: mortendk commentedComment #19
mgiffordI just looked over the code...
@mortendk this is going to be just way easier for folks to understand. Great job!
Comment #20
star-szrTestbot?
Comment #22
Fabianx CreditAttribution: Fabianx commentedTagging novice: Someone needs to fix the syntax error in node.module.
Comment #22.0
Fabianx CreditAttribution: Fabianx commentedremove comment, add view mode
Comment #23
markhalliwell#1927584-118: Add support for the Twig {% trans %} tag extension was committed!!
Issue is now blocked by: #2047095: Remove $submitted from node templates
Updated issue summary.
Comment #24
stevectorThere is now a patch in #2047095: Remove $submitted from node templates
Comment #25
minneapolisdan CreditAttribution: minneapolisdan commentedRerolling patch from #18 and fixing syntax error.
Comment #26
Les Limchanging status.
Comment #26.0
Les LimUpdated issue summary.
Comment #27
Fabianx CreditAttribution: Fabianx commentedTestbot, get to work!
Comment #28
markhalliwellOk, sorry guys. This isn't isn't blocked by #2047095: Remove $submitted from node templates (read comment I made there on why). It's now a dup of this issue.
Here's a review:
I'm confused. Aren't these the same thing? Why can't we just use
{{ node.comments }}
in the template file? Idk, just seems redundant.These need to be removed.
I think these should be, yes. Just not sure how this will actually impact
{{ content }}
.It's not in here, but the
- submitted
variable needs to be taken out too.There's no real need to separate the classes from the attributes. This is generally only done if we're manually adding a class via the template (ie: like clearfix... which we seem to be taking out?) or something. Since we're not, it should just be:
<article{{ attributes }} role="article">
See my comment in #1982244-26: Markup for: block module about the title stuff... we should probably change this to match.
Why are we usingAfter discussing this with @mortendk on irc, the W3 footer spec, says the author and date stuff should be in<footer/>
here?? Shouldn't this be<header/>
instead?<footer/>
Might want to put the title in
<header/>
too... idk, thoughts? edit: see Bartik's node.html.twig file.The submitted stuff needs to be expanded out using the new
{% trans %}
tag. See the latest patch in #2047095: Remove $submitted from node templates.This should probably be
{{ content.attributes }}
... but... idk, wouldn't that mean we would have to make the actual content{{ content.content }}
? That seems messed up, thoughts?I think we can get away with just
{{ node.comments }}
here. Or do we really, really want to have comments be a TLV (top level variable)?Take this out.
Same "drillable" idea here as above.
This just needs to be refactored using the new
{% trans %}
tag as above. EDIT:This should also be changed from<header/>
to<footer/>
. After discussing this with @mortendk on irc, the W3 footer spec, says the author and date stuff should be in<footer/>
.Comment #29
markhalliwellPs... can we also PLEASE remove
.title
from the<h2/>
tag??? There should only be one<h2/>
per node anyway, it's really annoying.Comment #30
mortendk CreditAttribution: mortendk commentedyes we can kill it right now kill kill kill kill kill kill kill
Comment #30.0
mortendk CreditAttribution: mortendk commentedUpdated issue summary.
Comment #31
Jeff Burnz CreditAttribution: Jeff Burnz commentedActually there is a good reason to separate class attributes - to make it easy for themers to add classes without preprocess function or knowing they can break out attributes in this way. Node, page, comment and block are very likely candidates for this sort of class adding.
Removing .title selector from h2 goes against our own coding standards - so should be .node--title or something like that, we should not have to do stupid descendant/child selector to style a node title, .node > h2 {} is uncool in anyones book, we should use .node--title {}, which is scalable/more robust, aka what if later some tricky dev adds a new inner wrapper to node, then child selector is broken, uh oh, we relied on html structure for presentation, bummer for you.
You can say "should have one h2 per node", not quite right and not best practice in html5, if I section, say on fields, then html5 headings reset, I can have as many h2's as I want in as many sections as I want. We can't assume how html5 is going to be used by the dev, or in common practice in 3/4 years time.
Comment #32
mortendk CreditAttribution: mortendk commentedwe can add .title whatever into bartik but not in stark - thats not going against coding standards & thats what have been disucssed the last year to have a drupal core that is clean of " as much as possible" (https://drupal.org/node/2008464#principles)
if you want that kinda classes like h2.title then they should be added into bartik, thats gonna be turned into a prober basetheme.
Comment #33
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK, I see those docs now, they seem a bit at odds with the CSS standards, hence some confusion, but yeah, I am pretty keen about breaking out those attributes, that looks to be within scope of the principles aka 'visibility within templates' etc. Don't you mean discussed for the last 8 years? ;)
And yeah, I am all for getting rid of as many generic classes like title, content etc as possible.
Comment #33.0
Jeff Burnz CreditAttribution: Jeff Burnz commentednode.twig.html -> node.html.twig
Comment #33.1
dead_armAdd related follow up
Comment #34
babruix CreditAttribution: babruix commentedRerolled patch.
Applied changes from comment #28.
Function comment_node_view is not more exists - should we add it back?
Comment #35
joelpittetI don't think this is needed.
Added a . to the node-class by accident.
This is strange we are checking for content.links and printing links.
Comment #37
babruix CreditAttribution: babruix commentedComment #39
droplet CreditAttribution: droplet commentedWhy not node.url ?? I see no reason to add a custom var. It isn't consistently.
Comment #40
joelpittetCouple more thing, and I'm with @droplet on the node.url thing it seems to have more context but curious your thoughts there as it's a bit subjective:
Also could you provide an interdiff with the changes?
This should be $node->id() still.
Missing a space after attributes.class inside the {{ }}.
And the {{ attributes }} needs the space before it remove because it prints a space automatically before each attribute.
Comment #41
babruix CreditAttribution: babruix commentedApplied suggestions from comment #40.
Uploaded interdiff.txt against #25.
Comment #42
joelpittetThanks for the interdiff @babruix! On the classes need space I think you went the other way and remove the space. Should look like this, like all the rest of them:
{{ attributes.class }}
+++ b/core/themes/bartik/templates/node.html.twig
@@ -71,23 +71,16 @@
+
And could you remove extra space after it as well?
Comment #44
star-szrNot really a feature IMO :)
Comment #45
babruix CreditAttribution: babruix commentedComment #47
star-szrTagging for focus.
Comment #48
mortendk CreditAttribution: mortendk commentedComment #49
Dragan Eror CreditAttribution: Dragan Eror commentedWill check this one...
Comment #50
Dragan Eror CreditAttribution: Dragan Eror commentedTested locally, all tests passed.
Bot retest...
Comment #51
Dragan Eror CreditAttribution: Dragan Eror commented45: node-2004252-45.patch queued for re-testing.
Comment #52
dasjoi can't see a reason why author + submitted should go into the link-wrapper & why they should only appear when content.links is set
Comment #53
Dragan Eror CreditAttribution: Dragan Eror commentedWe had discussion (@dasjo, @rteijeiro, @LewisNyman and me) at Drupal Developer Days in Szeged and agreed this patch need few more improvement to improve logic and D8 CSS coding standard. I will make these improvements...
Comment #55
mortendk CreditAttribution: mortendk commentedthe role is allready added in the preprocess
if theres concencus about moving out all the classname generating of the preprocess #2217731: Move field classes out of preprocess and into templates that would be another thing to consider maybe getting into this patch as well (or as a follow up, to get the ball rolling)
Comment #56
LewisNymanI don't think we would need a double dash here. Just one dash is fine. Maybe node-id-{{ node.id }}
I think we discussed already to change this to underscores
Comment #57
dasjoi'd be fine with
node--ID
as from my perspective that's a modifier class.https://drupal.org/node/1887918#extend
Comment #58
Dragan Eror CreditAttribution: Dragan Eror commentedChanges done in this patch:
{% if content.links or author or display_submitted %}
. (in node module and bartik theme)class="link-wrapper"
from footer. (in node module and bartik theme)class="node--submitted"
to use underscores. (in node module and bartik theme)class="node--{{ node.id }}"
to use double dashes. (in node module and bartik theme)Node IDs are not removed because some test require them. For this I suggest follow up task, which will cover tests also.
This patch does not pass tests, I tried to fix them too but after spending too much time I give up...
Comment #59
Dragan Eror CreditAttribution: Dragan Eror commentedComment #60
acouch CreditAttribution: acouch commentedI re-rolled this as it would not apply cleanly. The tests that failed in #45 work locally. I also made the spacing consistent between the content.links the module and bartik files.
Comment #62
nikhiltri CreditAttribution: nikhiltri commentedI corrected the XPath syntax to reflect the current HTML structure of the body tag.
Comment #63
visabhishek CreditAttribution: visabhishek commentedChanging Status to review.....
Comment #65
scor CreditAttribution: scor commentedwe're not removing the submitted variable in this issue, in fact it is used further down in the template.
we're missing the corresponding variable name change in template_preprocess_node(), which is why the RDF tests are failing.
Comment #67
chrisfromredfinI've addressed the above to comments in 65. Somewhere since the patch in 25 we lost that url variable in template_preprocess_node; I also fixed the missing documentation error.
Comment #68
scor CreditAttribution: scor commentedI believe we want to get rid of node_url and use url instead.
Comment #69
chrisfromredfinComment #70
mortendk CreditAttribution: mortendk commentedwhy are we keeping id="node-{{ node.id }}" ?
cant we get rid of that once & for all :)
Besides of that it looks good to me, think if we need some screenshots for bartik, seven & stark
Comment #71
rteijeiro CreditAttribution: rteijeiro commented@mortendk: Check @Dragan's comment #2004252-58: node.html.twig template. Some tests need node id so if we get rid of it, tests fail.
Comment #72
mortendk CreditAttribution: mortendk commentedrename the .content to .node--content, to keep up with our css standard.
I have tried to change the test, so it goes after the article class="node--$id instead of article ID="node--$id
Comment #73
Dragan Eror CreditAttribution: Dragan Eror commentedAs far as I've noticed there is "big" mess around Drupal 8 because making decisions per task. Guess "id" attributes are not removed from all twig files, but we want to remove them? Why not creating new META issue with related issues per twig file, so we can make it unified and standardized avoid to miss it somewhere?
Comment #75
mortendk CreditAttribution: mortendk commentedremoving all of the id's in one big giant patch will become a cluster F. - We take em one by one, template by template & optimize on each element as we go, remoiving them all in one would guarantee that we would end up not beeing able to do anything.
But yes it would be nice to be able just to clean out all the cruft in one go ;)
Comment #76
chrisfromredfinFor what it's worth my buddy and I beat on trying to get that Multilingual test to pass using the class name instead of the ID for like 2 hours at the Chicago sprint and we had no such luck. Seems like it should totally work, but I'd love to see it work if someone knows XPath syntax better than I...
Comment #77
Dragan Eror CreditAttribution: Dragan Eror commented@mortendk no one giant patch... Separate patch for each task... But one "META task" for all smaller tasks.
Comment #78
mortendk CreditAttribution: mortendk commentedI dont think it would be a productive idea to create a new task every time a test fails, we "just" have to dig in and battle the test monster or find someone whos better at writing tests, then we can focus on the markup / classes :)
Comment #79
mortendk CreditAttribution: mortendk commentedid's changed to classes for testbot ...
Comment #81
Manuel Garcia CreditAttribution: Manuel Garcia commentedNew to this issue, but want to lend a hand. Here's a petty attempt to fend off the test bots...
node title test was missing a dash on the node--ID class (i think)
I've checked the other failing tests... they look fine to me, perhaps we are missing something on the twig file?
Comment #83
mortendk CreditAttribution: mortendk commentedtest is updated, so we dont need the id="" anymore :)
Comment #84
jibranIf we use hidden formatter for title in node display then
template_preprocess_node
shows a warning. Can we fix it here? It only requires simple isset check.Comment #85
visabhishek CreditAttribution: visabhishek commentedpatch updated as per #84.
Comment #86
rteijeiro CreditAttribution: rteijeiro commentedIt looks that something happened with "submitted" content. Take a look at the screenshots:
Before
After
Comment #87
mortendk CreditAttribution: mortendk commentedok im on it & gonna put in the screenshots as well
Comment #88
mortendk CreditAttribution: mortendk commentedseperated out the links & moved them above the comments, the "add a comment" link was placed belov the comment form - which makes no sense ;)
fixed the submitted issue as mentioned in #86
Comment #89
mortendk CreditAttribution: mortendk commentedHeres the screenshots for patch #88
Bartik
Bartik teaser before
Bartik teaser after
Bartik node before
Bartik node after
Stark
Stark teaser before
Stark teaser after
Stark node before
Stark node after
Comment #90
mortendk CreditAttribution: mortendk commentedforgot to fix the classes to follow our own css naming standard (https://drupal.org/node/1887918#sub-components)
no changes to screenshots *phew*
Comment #91
mortendk CreditAttribution: mortendk commentedupdated the issue with proposed changes & template
Comment #92
markhalliwellWhitespace
This should be
{{ url }}
, not{{ node.url }}
.I would say instead:
author: The node author user entity, rendered using the "compact" view mode.
This doesn't make sense. It's not apart of the previous sentence.
Just to verify, are we intending to get rid of the
clearfix
class on both templates? I can see why Stark wouldn't, but shouldn't Bartik?Also, semi-related, does this inadvertently leave a trailing space if there are no
{{ attributes.class }} present to print?
Space after
,
please.The if statement here was lost. Was the change from
<footer>
to<div>
necessary?Comment #94
markhalliwellComment #96
mortendk CreditAttribution: mortendk commentedFixed the test & the comments from #92
the test freaked out that i split the comments out of the content & frankly that is another task we should take when we get into comments (yes postponing) for stark
Comment #97
markhalliwellNot all the comments in #92 were addressed:
Also, please provide interdiffs so it's easier to see what has changed.
Comment #98
star-szr#92.2 - I think part of the point of this issue was to remove the node_url variable and just have Twig call the url method for us by using {{ node.url }}. So I think with the latest patch we can redo that change and remove the 'url' variable from preprocess.
In fact I'd also suggest that 'label' could very likely be {{ node.label }} if we wanted.
#92.5 - This patch should probably make only very minimal changes to Bartik, at least that's what we've been doing for other markup change issues. As for the empty space if attributes.class is empty, some of the approaches taken in #2187113: Incorrect usage of attributes in twig templates resulting in possible duplicate attributes. might be valuable, in this case the |merge filter. In general though I think we need to figure out what to do with attributes in core because adding variables both in preprocess and in the template gets messy pretty quickly.
Comment #99
joelpittetNeed spaces on either side of these variables to make the concat/normalize actually work correctly.
Same here add space on the :node-class value.
80 chars
Comment #100
mortendk CreditAttribution: mortendk commentedfixing of space 80 char etc - now sleep
Comment #101
mortendk CreditAttribution: mortendk commentedComment #102
LewisNymanLooking good, some minor questions:
This is very minor but I wanted to check it was intentional we're removing the blank line before the closing tag, we have a blank line after the opening tag. Same thing in the Bartik twig file
Do we need to specify the view mode in the documentation? Couldn't this easily change and become false? I haven't seen us do this anywhere else. How about just "The author of this node"?
Comment #103
visabhishek CreditAttribution: visabhishek commentedupdated as per #102.
Comment #104
markhalliwellBoth Core (Stark) and Bartik templates are now no longer in sync with these documentations.
It should be clear that this is the node author entity rendered as the "compact" view mode. Yes, it should be explicitly defined here (by default). If a theme overrides a template (and view mode) then it's their job to change the documentation in their overridden template.
Furthermore, I know it's sad that we're rendering before it even gets to the template, but because we don't have a
|view_mode()
filter in twig, this is currently unavoidable.We must be clear what is being passed to the template, that is the point of documentation: to explicitly define what is being passed to the template; "The author of this node." is simply too ambiguous. Is it the entity, rendered markup, just the name, what?
Also, when someone raises a question, it's best to discuss it before making the change. Regardless of what the outcome is, please make sure that the 80-character limit is enforced and that it's a complete sentence. The the punctuation doesn't fit, just move the last word to a new line... please, don't remove it.
Same here. This should just be restructured so that "rejected." is moved to the next line. The following sentence should also be on that same line and reformatted so it fits within the 80-character limit.
Again, I'll ask: are we intentionally removing the "clearfix" class from core's (stark) template?
I'm not entirely sure, but I think we lost the core (Stark) template's division of comments from content (#90). @mortendk, was this intentional on your part?
Whitespace.
Missing period.
Again, I'll ask: are we intentionally removing the "clearfix" class from Bartik's template?
Again, I'll ask: are we intending to change this tag from
<footer>
to<div>
in Bartik's template?Comment #105
markhalliwellPS: there's already a "Needs issue summary update", but can we please get some clear direction about what this issue is trying to accomplish? There's some scope creep here.
If you don't use Dreditor, you can manually follow the Issue summary instructions
Comment #106
droplet CreditAttribution: droplet commentedalso, don't remove the args. @see: #2149649-64: Entity rendering/theming does not use the active entity language to render links
Comment #107
mortendk CreditAttribution: mortendk commentedComment #108
mortendk CreditAttribution: mortendk commentedComment #109
mortendk CreditAttribution: mortendk commented@markcarver:
changing the footer to a div for the .link-wrapper, cause its not a footer
clearfix is removed from stark, but will be in bartik, its layout behaviour, so it has nothing to do in stark.
content comments & splitting them in stark is not the role for this patch - i would suggest a follow up patch later if we want stark to do more than it does today
... now trying to do an interdiff... stand clear
Comment #110
mortendk CreditAttribution: mortendk commentedinterdiff + new patch added
Comment #111
mortendk CreditAttribution: mortendk commentedkicks bot
Comment #112
RainbowArraySorry, I don't get this. The header element for the article is being removed, and part of the header element is being turned into a footer element? It made a lot more sense to me before. The header and footer elements are scoped to their parent element. So it makes perfect sense to have the page title and the submitted info in header, and the links that appear below the content in a footer. I don't think I agree with those changes.
Comment #113
mortendk CreditAttribution: mortendk commentedupdated the issue summary - hopefully it make more sense now
Comment #114
mortendk CreditAttribution: mortendk commented@ mdrummond
submitted info belongs in the footer tag:
http://html5doctor.com/the-footer-element/
Having a header around a h2 to make little to no sense, thats just to add in a header tag cause we can ;)
Comment #116
derheap CreditAttribution: derheap commentedThe template looks ok for me so far.
Just a few questions:
* Why do we need the node-id class? Is it nessesary for the contextual edit menu? Other usecases?
* Do we need the div around the real content?
What are the use cases?
We can style alle text content via the body style. All exeptions are in special divs and can be treeted special (author, headline, submittet, links).
The conent consists of field which are wrapped in div with field classes.
This div is one of the first I am removing in every single node template …
I would like to get rid of it forever.
@mdrummond
The use of the footer element is correct – the element (footer) name is misleading.
Comment #117
mortendk CreditAttribution: mortendk commentedanother cleanup in this patch:
* added is-state classes & corrected all the classnames to follow the code standards
* changed the test for is not page to is teaser, cause it makes more sense to read
Comment #118
mortendk CreditAttribution: mortendk commentedComment #120
mortendk CreditAttribution: mortendk commentedfixed the test and went back on the {% if not page %} as it might gonna be out of this scope
my thought were that its not logical what {% if not page %} actually does i would suggest to test against the viewmode instead, as we actually are.
it make immediately sense what it is that the code is doing & shows more possibilitys - maybe that should be in a follow up issue ??
Comment #122
RainbowArrayI don't want to derail this with a header and footer argument. But just so people don't think I'm daft, here are the actual definitions of header and footer:
HTML5 Doctor is a good site with quality advice, but its examples and advice are suggestions, not the spec itself. What the spec says in the footer definition is this:
I think it's perfectly sensible to consider a heading and byline introductory material that could go into a header. A footer for the submitted info is fine too, but putting links in a footer element makes sense too. So if you want specific grouping elements for those, then putting submitted in a header and links in a footer could work just fine.
However, it's important to note that the heading only shows up here in the teaser view. So in all other situations you'd have the submitted info in a header element all by itself, with no heading. That doesn't really make a lot of sense.
I don't think there's an absolute need to use header and footer elements here either. You could just use divs for the submitted info and for the links, and everything still works just fine. The header and footer are optional elements. I'm not aware of any ways that they actually have an effect on how the page is processed, either by search engines or screen readers. So it might be simpler to just go with divs for the submitted info and the links. A lot less arguing that way!
Comment #123
scor CreditAttribution: scor commentedwhy leaving an empty line here?
aren't we changing the behavior here? the new if statement means that if we have author and not display_submitted, we will still display the submitted and author, looks like a regression to me.
Regarding
$variables['content_attributes']
, it was added to node and other entity templates in D7 for consistency in order to have:-
$variables['attributes']
for the element wrapping the entity output,-
$variables['title_attributes']
for the element wrapping the title, and-
$variables['content_attributes']
for the content wrapping element.See block.html.twig for an example of where these are placed in the output. The RDF module makes use of 'attributes' for the entity wrapper, and 'title_attributes', but not 'content_attributes'. 'content_attributes' was added to be consistent, and in case other modules would need it. I don't know if there are module which make use of content_attributes.
Comment #124
mortendk CreditAttribution: mortendk commentedphew another cleanup:
Lets stop this node--id madness
@derheap #116
.node--$id class
you are totally right about the node--$id, i have been theming sites for 8 years now, and i have so far only once used node--$id, and i set that as a class ;) I havent heard about anybody that have ever used the node--id for anything, it smells & looks like legacy + What-if cruft.If we follow the codestandard & our guidelines this class do not fall into the 90% rule, and its dead easy to put in your self as a themer & why i wanna keep the
<articler class="{{ attributes.class }}"{{ attributes }}>
so its intuitive for the themer to see where a node--$id can be added.Unless somebody can raise strong case for keeping
.node--$id
even as a class -I suggest that are reaslistic AND remove this abomination of useless information - yes i feel strongly about this... ;)
Wrapper on content
The wrapper is there to have some structure for the inner workings of the node
node__content
the first time you open up the site + the {{ content_attributes }} is there for modules to ba able to add stuff here, even that i never have seen anything in that placeholder as @scor also comments on in #117What would be nice was some better documentation for this
{{ content_attibutes }}
cause it feels & smalls like cruft - but lets do that in a followup issue.header vs footer ... wtf HTML5
Its seems like we can open up an epic bikeshed over the use of
<footer class="node__meta">
vs<header>...<div class="node__meta">
Lets try not to do that ;)As I see it: Bartik & stark is completely fine with having different markup, Bartik is a theme that have a design on top of it, where stark is just starknaked. In bartik the design have the author info as part of the header (& why mdrummond is *kinda* right ;) Where stark dosnt care and have it seperated out, why that can be a
<footer>autor</footer>
else i would move that we dont change in the markup as it is now change the classnames to follow the standards & we can do a follow up issue, where the epic battle of author info can start.this patch:
Removed the node--id competely (gasp!)
Added the preview, sticky, unpublished, preview classes to be modifier classes for node,
node--sticky node--unpublished node--preview
Comment #126
joelpittetTotally for removing node-5 from class and/or id. It just gives false hopes to themers that id is going to be the same between dev/stage/prod environments. Though the id's look unique they are not! The uuid is the unique key but that's nasty and would rather not see that in CSS.
Those special cases where a themer *thinks* they need this, it can be easily added back in. Though it would be recommend to use the node type or a custom class from the UI for that node if necessary... something for contrib to think about.
Thanks for doing this @mortendk!
Comment #127
camoa CreditAttribution: camoa commentedVery nice Morten, It looks good I did a tiny comment on line 37-38 of node.html.twig. I believe the documentation is wrong since you made the type class node--type-[bundle]. Not sure if I am right, though.
Comment #128
Jeff Burnz CreditAttribution: Jeff Burnz commentedIndeed, looking very good!
Is it within the remit of this issue to clean up the output, especially of user picture / author, or is that in some other issue etc, for example you always get article wrapper even if the user has no picture, why we need that article wrapper I have no idea, I would do something like this:
now you get nothing if there really is no user picture and no article wrapper when there is, just the field wrappers.
Comment #129
mortendk CreditAttribution: mortendk commentedFixed the documentation & the test, added in the {{ attributes|without('class') }} now that the without on attributes came into core.
@jeffburnz lets open up another issue about the user picture and fix that seperate, im pretty sure its gonna open up another can of worms ;)
testbot: get to work
Comment #130
mortendk CreditAttribution: mortendk commentedComment #131
Jeff Burnz CreditAttribution: Jeff Burnz commentedRight you are: #2247677: User picture improvements in node template
Comment #132
mortendk CreditAttribution: mortendk commented@jeff awesome - lets clean that out :)
... sooo can we get an RTBC on the node template ?
Comment #133
joelpittetThis is looking really really good. Just some nit picks and I think I could RTBC this.
Can this variable be named created_time so that we are consistent with the naming pattern of _ separating words?
These lines should be node--view-mode-...
Since there no clearfix or id, I'd like to see this just {{ attributes }} without|without() so there is no chance of empty class="" attribute if the classes are removed in preprocess and a cleaner template.
This will duplicate class="", it needs |without('class') or the class needs to be added in preprocess.
This looks more consistent between the templates, thanks for moving it up.
Same note as before.
class should be split with |without('class') or you could get duplicates class attributes here. Good idea to dump the classes in after the clearfix too so they don't get lost.
Comment #134
Jeff Burnz CreditAttribution: Jeff Burnz commentedI understand the motivation here (clean template) but it's also more opaque/abstract for new themers. I tend to think if you're at the level of removing classes in preprocess you know about attributes entirely and understand what's going in the template, in other words the advanced themer does not need hand-holding, but the newbie does a little bit. Front end developers already have the whole power of the attributes system at their fingertips, whereas the new kid on the block just wants to get their class added.
Either way I'm not arguing really strong on this point, but I do tend to agree with this pattern of breaking out the classes in this particular template, mainly because like block its so common to override.
Comment #135
joelpittet@Jeff Burnz I completely agree and to further that point the first theme a user sees is Bartik, so far we've been putting those examples of how to split attributes out in that theme. We are trying to keep core templates as clean as possible. Does that explain my reasoning better?
Comment #136
mgiffordI looked through the latest patch to see if I could find any accessibility problems. Looks good!
Only potential problem I can see is the removal of
id="node-{{ node.id }}"
but that's only a problem if there is something using it. I don't know of anything off hand that does though.Comment #137
Jeff Burnz CreditAttribution: Jeff Burnz commented#135 - sure, I'm not strongly for or against either approach.
Comment #138
mortendk CreditAttribution: mortendk commentedpatch:
* fixed the {{ attributes|without('class') }}
* updated the inline documentation so its now understandable what the variable name is not foo but node.foo.
Theres no reason at all to have concerns about removal of the node-id -its a badpractice + nobody is using it. If a theme wants to use #id they can easy add it in with {{ node.id }}
created_time we should fix that world wide imho - but i would suggest that we take that when we clean up the comments (where it also exist) we get the node patch in, so we can start that work - Cause its been almost a year of nidpicking, so any bikesheeding we can take at the bar (im buying)
Comment #139
mgifford@mortendk I remember talking to you about that in Portland, and from a CSS/HTML perspective, absolutely.
However, WAI-ARIA attributes like aria-describedby use ID's as does JS. I don't think that the JS is going to be an issue and can't see where ARIA would at this point either.
I don't think there are any instances where it is needed in this template, I just raised it as something to be considered as there are places where they are necessary for adding semantics.
Comment #140
mortendk CreditAttribution: mortendk commented@mgifford cool so it seems that we agree :)
Comment #141
mgiffordYup. Looking forward to seeing this in Core.
Comment #142
mortendk CreditAttribution: mortendk commented/me dances ... and yes im looking forward to get into the comments and clean that up as well :)
Comment #143
mortendk CreditAttribution: mortendk commentedyo bot get to work ..
Comment #144
joelpittetFew more, thanks for the ones before and I agree createtime can change globally elsewhere.
'node ' needs a space on both sides to be correct. so ' node '. Unlikely but would also match "article-node ..." or something.
Space after the @class, commas.
Add a space before 'node--type-' so ' node--type-'
Need a period at the end.
I don't know about this node. being prepended because that is not in the coding standards. Maybe Cottser can confirm if this is right or not?
80 chars.
Remove the space before the attributes here.
Comment #145
mortendk CreditAttribution: mortendk commentedheres more for you joel
Comment #147
joelpittetGetting there, can we revert the node. additions to the docblocks so we are consistent with https://drupal.org/node/1823416 ?
Though I agree this is clearer maybe we can open up a twig docblock issue to discuss how to be consistent and clear as we discussed in the hangout?
Just realized these have a few too many ---s
Whoops this one needs the space before the class. Attribute renders with a space before because all attributes have that, but the values don't prepend a space.
Comment #148
mortendk CreditAttribution: mortendk commentedfixed the documentation things & changed the test so it works with
.node__content
Comment #149
camoa CreditAttribution: camoa commentedDie ID, Die!!
Comment #150
joelpittetFound some more missing bits, please review the interdiff to make sure these make sense.
Comment #151
mortendk CreditAttribution: mortendk commentedComment #152
mortendk CreditAttribution: mortendk commented@joelpittet sweet yup it looks good.
ooh good catch - yes its a modifier for node
i dont think i can put this to be RTBC, cause of looong work on this patch, its itching in my fingers though.
but to me it looks ready to roll (i sure hope)
Comment #153
joelpittetNeeds before and after screenshots/manual testing of node pages and teasers in stark, bartik and seven.
Comment #154
mortendk CreditAttribution: mortendk commentedHeres the 12 screenshots that shows that the patch is not changing the visuals - to not overload the page, im not embedding them on the page - this page is long enough all ready.
Comment #155
joelpittetDid 3 things.
Fixed the margin around 'submitted' element shown in the screenshots above. There is no P tag so it will now have no margin on stark, but I've re-applied it on bartik doesn't care because it removed that p tag.
I moved the RDF stuff to the template for both the author_attributes and metadata. Need an RDF person to take a look to see if this is cool or a better way to do this?
And used the trans tag to get submitted string into the template.
Comment #157
joelpittetThis should fix those exceptions and errors for rdf in the last patch.
Comment #158
mortendk CreditAttribution: mortendk commentedthe data comes from the user and can in principle be whatever the editor have added into the compact view mode right? - so to be nidpicking, is author_picture the right name or should we instead grap that user image directly - and not use the entity ? that would make more sense, but maybe that should be a seperate issue...
if we choose to create another issue, then its screenshot time again ;)
Comment #159
derheap CreditAttribution: derheap commentedAbout the author_picture:
Yes, another issue please. Is #2247677 the right place for that?
Comment #160
Jeff Burnz CreditAttribution: Jeff Burnz commentedshould be {{ author_picture }}
Will wait for scor to discuss, however for now its there but no comments in the docblock about what it is or what it does etc.
Comment #161
Jeff Burnz CreditAttribution: Jeff Burnz commentedI am sure you guys have some fang dangled argument about this, but honestly I do not get this idea of no classes on titles in block and node - you force element selector usage and deep nesting of selectors, even for silly simple things like wanting the node title and node content h2 to have a different font. h2 is an element, so give it the component level selector node__title, without I have am forced to:
.node h2 {}
.node__content h2 {}
forgive me, that that just ain't BEM style, so no, We aint rockin' it (not contagious), dis jus monkey business (outrageous...)... :/
Comment #162
RainbowArrayI'd agree that there should probably be a class on those h2 elements. Maybe that's hidden in title_attributes, but if not, would be good to get that in there. Probably node__title. Might want node__link on the anchor inside too.
Comment #163
mortendk CreditAttribution: mortendk commentedSeriously guys...
Forgive me but can we put your concerns into a follow up issue and lets get this patch in ?
its extremely frustrating that we have used almost a year to get this in - Using almost 1 year on cleaning up the node.html.twig file is absurd - seriously it should be done within 2-3 weeks
I personally have strong objections about having a preprocess that creates hardcoded classes (and using attributes to put that in), but those are gonna be in a followup issue - why, Cause we need a cleaner node to get in to be able to move forward.
At this speed were barely walking.
Comment #164
joelpittetI can revert and split the submitted move to trans block in the template to a new issue if that helps any?
Comment #165
mortendk CreditAttribution: mortendk commentedsorry if i seem to rant here but (thats the nice way of saying rant alert)
It seems like the discussions that we have had the last 2 years dont matters anymore & were desperately trying to put on classes on everything we can find in the case of: ooh no the themer have to add in the classes s/he needs.
Seriously wasn't there an agreement of "start with nothing & then only add on as needed" ? - Did we forget that or do we have to remind ourself of that every 2 weeks ?
-> https://drupal.org/node/2008464#principles
We can put whatever we want on Bartik as we sees it (it can drown for all that matters in divs & classes)
But wasnt the idea behind stark to keep it as clean as possible - this is ment as the minimum base. NOT the base thats everything you can possible think about. Else what were gonna end up with is just another 4.5,6,7 Drupal version with a
{{ wooho }}
Else we need to discuss the idea of having a "Drupal Base" as a basetheme, then we can add in all the things that are atm belived to be the best practive (and that we know will be out of date in 12 months)
Sorry guys but we need to keep focus on the bigger picture here - and i feel that we are not.
Comment #166
Jeff Burnz CreditAttribution: Jeff Burnz commentedMorten - I am not raising some discussion about the principles, or that this should be an attributes class (it should not IMO), however those principles do not discount the raising of this specific issue - there is nothing in those principals about adding specific classes to one element, but not others - they're a general guide, which is subject to interpretation.
For example: Consistency: Do the same things everywhere, follow patterns. Not having an h2 class is incosistant with other elements in the template, here's the most obvious example:
<footer class="node__meta">
- here we have a semantic sectioning element that does not actually require this class to be styled via CSS, because you can do:.node footer {}
.The "consistent" pattern being established here is BEM, but we're not being consistent with it. If we look at the template as a whole, we have a BEM class on practically all elements, except one - and one that is highly likely to be repeated in the actual node content (h2). By not having that class we loose the advantage of BEM - flat specificity and maintainability.
What is the point of adding these BEM classes in the first place if those are not the goals?
Why add
node__content, node__meta, node__links
etc, none of which are actually required other than to provide convenient hooks for themers?We can consider: Build from use cases: Think about the 90% of use cases - It would be my interpretation that 90% of the time users would want to specifically style titles and headings in nodes, I'm not saying the can't, but they would be forced to override the template or use preprocess if they want to conform (be consistent) with BEM, or rely on HTML structure and then having to deal with specificity issues - again that feels very inconsistent with the BEM concepts and patterns being established in our core templates.
These are fair questions that deserve a fair and reasonable technical discussion, I would kindly ask you to not rant at me every time I raise such issues, even if this takes more time.
[edit, added a code tag]
Comment #167
derheap CreditAttribution: derheap commented@ Jeff Burnz
@mdrummond
Let us please remove stuff – according to the agreed guidelines – and get this RTBC.
Stark should be as clean as possible – the ground to build on. Not the ground to start removing stuff.
If you need an node_title for your styling – fine, add it to your template.
I dont see a usecase für
.node h2 {}
. As nodes are the content of the site, most h2 will be node titles. So they are the rule, not the exeption.And
.node__content h2 {}
should not be nesseccary, as there should be no h2 inside node_content.And even the classitis rich Bartik does not put a class on the h2 of a node.
So don’t let us do worse than Bartik.
For me this issue is not going far enough. I would like to remove
<div class="node__content"{{ content_attributes }}>
too.But that might be another issue.
So lets close this can of worms and get it in core.
For everything else please open another issue.
Comment #168
Jeff Burnz CreditAttribution: Jeff Burnz commented#167, generally I agree with this, but those hooks are pretty handy for 90% of use cases so I'm not sure we need to throw baby out with bathwater. Top my mind they mostly meet the principle of 90% use case - e.g. node__meta lets you drill down to field type image and img without too many problems later on, etc etc (like not bleeding styles into user template).
"there should be no h2 inside node_content" is not quite right, in html5 you can start any new section with whatever heading level you want. The point of BEM is to be able to largely ignore such structural concerns.
It's not a can of worms, it's not that serious imo, its a simple yes/no should we agree to be inconsistent or not, you guys are making this a big deal, I simply asked WHY it is like this, and so far Morten sites some principles and ranted on something about Bartik and you want to rip everything out, sorry but I am not quite sure why either of you would have such strong reactions to someone simply pointing out a very obvious inconsistency.
Comment #169
scor CreditAttribution: scor commented@joelpittet #155: "I moved the RDF stuff to the template for both the author_attributes and metadata." <-- looks good, thanks!
Comment #170
mortendk CreditAttribution: mortendk commentedWhat im "ranting" about is our process that is completely broken & constantly stop patches - and that is draining energy. This patch is almost a year old for crying out loud. When we constantly stop on small details - i really dont care that much about node__title or whatever, then were stopping ourself to move forward.
So to take it on a technical level using selectors is not against our css manual
Using a h2 for a title and remove the old
.title
class makes sense cause it makes the template cleaner and its really easy to target with a.node > h2
not to mention that this h2 is only a thing we have in the teasers view (out of the box anyways)Its been raised many many times by many a themer "why dont we just use the h2 tag" - that imho makes sense, yes it also make sense to have a node__title (same thing as footer vs div etc) but should we bikesheed it for weeks, and stop a patch from getting in.
as a sidenode we are building on smacss with some inspiration from bem
If you guys really really really think that not having a
.node__title
is a blocker for this patch (really ?) Then create the patch so we can move forward.Lets not make perfect the enemy of good.
Comment #171
RainbowArrayThe problem stated for this issue is that we want to cleanup node.html.twig so that it follows CSS coding standards.
Good goal.
Here is our CSS architecture guidelines: https://drupal.org/node/1887918
Best practice one is to avoid reliance on HTML structure in CSS rules. We can only do that if we have the classes necessary in HTML to allow us to do that.
I understand your frustration, and where you're coming from Morten. I am just as on board as you with ripping out divs and classes that are unnecessary.
Frankly the object oriented CSS principles we're using for Drupal 8 are new to me. I've typically based my CSS on element selectors. I'm trying to learn this new way of doing things, and as I'm learning about it, it does make sense.
We can certainly create a follow-up issue to deal specifically with the node teaser title. (Now I'm thinking we may want a class of node--teaser__title and possibly node--teaser__title__link to more accurately reflect when this would appear.) However I do think that figuring this out is in scope for this issue, which is getting the markup for the node template to match up with our CSS guidelines and thus our CSS architecture.
My understanding is that there has been an agreement to use these SMACSS/OOCSS/BEM principles in Drupal 8's markup and CSS. If so, then we should be putting that into place in our markup.
Comment #172
mortendk CreditAttribution: mortendk commented...so as a followup question here over my morning coffee & cursing over the issueque and reading up on jeffburnz points on not beeing consistent :)
stark naked ?
We agreed to keep stark clean right ? now we have added in
node__footer
,node__meta
etc to core, dammit thats against the principle so that should be ripped out and - are we are failing our own mission?bartik
Good old bartik should have all those classes
.node__title .node__footer .node__whatever
goes in there.A thing we know is that theres a huge wish for simple classes and a simple consitency in out of the box of drupal, even that some of us want a complete naked theme to work out from. that means a few basic classes, that is simple to remove (so they are in the tempate file).
To move forward. I would suggest that we add in the
.node__title
to bartik, keep that out of stark (as jeff points correctly out) and get this baby in.May i suggest that we then create a followup issue that can take on the discussion about how little or much stark is coming with, the role of bartik, the role of a drupalbase theme etc. ?
rolled a patch with the node__title added to bartik not to stark, adde the {{ author_picture }} as well + a really lame documentation ;) for {{ metadata }} which im not sure why we need that i the first place @joelpittet ?
Comment #173
RainbowArrayI think the issue is that this isn't consistent.
System/stark right now has the following BEM-style classes: node__meta, node__submitted, node__content, node__links.
The article element also has some node classes on it in system/Stark.
The only element in here that doesn't have a BEM-style class name in system/Stark is the node teaser heading and the link that appears inside it.
I don't know why it's ok for all the other elements to have those BEM-style classes, but how doing the same thing for this one element is the end of the world as far as having semantic markup.
If the goal is to strip every single class out of system/Stark markup, that's one thing. That's not my understanding of what we're trying to do with Drupal's default markup, and I'd imagine there are probably a number of others out there who also don't agree with that.
But if we're going to try to commit this patch that has BEM-style classes for most of the elements in the markup for Stark, it would seem to make sense to do that for the node teaser heading as well.
Comment #174
mortendk CreditAttribution: mortendk commented@mdrummond: As i see it about stark: We are trying to create it with small impact on the markup as possible. Just because we are using a bem style class naming on a couple of elements is not the same as stark have to add a class to everything, as the case is here with the h2
Theres no way to target the links unless there's a
.node__links
(it used to be .links).node__meta
make sense in the same way.node__content
as well make "sense" the H2 dosn't need to have that class, thats why we agreed afaik here in this issue back in july 2013 (comment #31+)(how many times did anybody use a h2.title ?)
If anybody sees the patch at this state to need more work then put the status to "Needs work" ! Else its green & been hammered on for almost 1 year.
We can alternative:
1. Add a patch that change whatever it is that "needs work" and is blocking the node to be cleaned up.
2. Accept the state of the current node in core and drop fixing it cause of nipicking.
3. Use another 3 months on this, just to see if we can get it "perfect".
4. Bikeshed over whatever we can find (and i can find a few things as well)
5. Add follow up issues to clean up whatever follow ups that might happen
Seriously we cant keep doing this "Best practice of the issuque" where its whomever is willing to fight the longest wins, its retarded and just drains us all out.
So what is it gonna be ?
- yes im totally pulling a trigger here, sorry for that (not sorry) ;)
followup issues:
* A cleaner stark (remove all node--foo classes etc)
* picture article fun as jeffburnz created.
* node-classes out of the preprocess into the template.
* an epic bartik 2.0
(edited: added in followup issue suggestions)
Comment #175
derheap CreditAttribution: derheap commentedI am not completely happy with the result, but I am fine with it.
As I have found not nitpicks in the interdiff I put it to RTBC.
All other stuff should be followup issues.
Comment #176
RainbowArrayLet's take care of the rest in followup issues. Incremental progress is a good thing.
Comment #177
Jeff Burnz CreditAttribution: Jeff Burnz commentedLooks good.
Comment #178
mortendk CreditAttribution: mortendk commentedComment #179
mortendk CreditAttribution: mortendk commentedComment #180
mortendk CreditAttribution: mortendk commentedHere is all the screenshots.
bartik + seven is completely identical as they should be.
Stark had a padding that was added to the submitted, cause it was wrongly wrapped with a
<p>
instead of a div - this have been changed to a div. Stark is not a design - so it gives no meaning to add in the<p>
margin that a browser would add to the tag ( margin: 1em before & after).Enjoy
Comment #181
mortendk CreditAttribution: mortendk commentedand there they are
Comment #182
joelpittet@mortendk re: #172 the
{{ metadata }}
is for RDF. It's like like{{ title_suffix }}
variable but actually named for what it contains;-)The reason it's needed is RDF was appending metadata to the
{{ submitted }}
variable which no longer exists. So I needed to create a new variable to append that data to.Thanks again for all the screenshots!:)
I'm cool with the stark 'look' different because it's no longer a
<p>
tag so that is expected and all the others look perfect from my quick review.So RTBC++ continues:)
Comment #183
camoa CreditAttribution: camoa commentedI agree with Morten, after so long, stop the patch for tiny details seems to be impractical. There is so much to argue in both sides of the coin, class or not class for title, but the big picture is: have a better node template.
I think that discussion in the latest 10 post or so goes against a lean approach to the topic. We need to know we have a good MVP here. Will it need more work as it is used in the future? most likely!! but right now, it needs to be released and tested.
Send that little bugger to commit morten, if anyone has a concern about a detail, there are ways to do it set in place(follow ups) but those are part of interaction that anyway should follow any patch release.
Comment #184
Jeff Burnz CreditAttribution: Jeff Burnz commented#183, the devil is always in the details. It's not really about a class here or there, it's a broader discussion about being consistent or not (either way, super clean or not), it was agreed to have that later.
Comment #185
mortendk CreditAttribution: mortendk commentedYup its about looking at the picture of ok so we are here now with the node - its a huge improvement of what core is now.
As jeff rightly points out, theres details that we need to be aligned about. Those details are there for moved to 2 other issues:
#2254163: node stark naked
#2254153: Move node classes out of preprocess and into templates
and the what do we do about the author picture :
#2247677: User picture improvements in node template
Comment #186
Jeff Burnz CreditAttribution: Jeff Burnz commentedWhat I've found is that when you do this without setting class as an empty array you will get a notice if you later attempt to add a class (e.g. in your themes preprocess_node function). Indeed, this is what is happening to me, for example when I do this:
$variables['author_attributes']['class'][] = 'node__author';
I get a notice (and the class does not print):Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect in...(my preprocess function snippet as above)
What I've been doing when setting up new Attribute() in preprocess is this:
$variables['author_attributes'] = new Attribute(array('class' => array()));
A quick discussion about whats going on here? Do we have a bug deeper in the Attribute system (different issue?). Right now, as it stands with this patch I can't add a class to this attribute.
I'm real hesitant to set this back to needs work but this does not work properly AFAICT unless I am doing something way wrong here, which I'm pretty sure I'm not.
Comment #187
joelpittet@Jeff Burnz I'm inches away from making that way way easier to deal with here: #2239945: Refactor Attribute class into one class
I also tried special casing 'class' which will work kinda but there are a couple of other attributes that use AttributeArray like 'class' does. Though I prefer that refactor one. I'd love is someone could take a stab or just tag team that issue with me, even if it's just a small brainstorm session on skype or something.
You are correct though, you do need to initialize class but you can do it like this too:
Detailed in the change record @see https://drupal.org/node/1727592
ArrayAccess::OffsetSet() deals with the AttributeArray instanciation at the moment.
Comment #188
Jeff Burnz CreditAttribution: Jeff Burnz commentedYep, I understand what you mean here, my main concern here is/was is that we are actually printing {{ author_attributes.class }} in the template, but you would have to guess that you need to initialise this yourself before you can use it, or find the change notice (which I did some time ago but actually forgot about it). My point was that new users will face a PHP notice for using something they think should just work based on what they're reading in the template.
From my POV I can let this slide and set it back to RTBC, since I know to do this, or is this rather bad TX, or just the way it is - I'm not strongly favourable one way or the other. If it can be fixed, which is what I think you're suggesting in #2239945: Refactor Attribute class into one class, then we can move on.
Comment #189
joelpittetSpeaking of TX/DX and attributes, I need someone to review this patch #2108771: Remove special cased title_attributes and content_attributes for Attribute creation
This avoids treating content_attributes and title_attributes variables different then all other attributes that are built out in the theme system. I tried to do $variables['attributes'] too which is also a special cased variable but that turned out to be a bigger challenge and if I can get those other two committed I can make a followup to do the $variables['attributes'] as well.
*shameless attributes plug*
Comment #190
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'm finding this is not always empty, e.g. in display mode full there can be no links but there are items in the array, upshot - empty markup:.
Comment #191
mortendk CreditAttribution: mortendk commentedoooh damn
Comment #192
Jeff Burnz CreditAttribution: Jeff Burnz commenteddamn indeed, is that something new, did this array suddenly change or something, I don't recall it being like that before (surely one of us would have noticed this), I just noticed it in my own template and went, oh shit, what-t/f is that?
Comment #193
markhalliwellCorrect. This should be:
{% if content.links is not empty %}
. Twig can handle checking if it's empty appropriately.Comment #194
Jeff Burnz CreditAttribution: Jeff Burnz commentedI don't think it's that easy Mark, because this uses a render-cache-placeholder, content.links appears to be never empty.
Comment #195
markhalliwellRender cache placeholders are a separate issue then and should be just another follow-up issue that needs to be created. Setting back to RTBC since Attributes already have their own issue as well.
Comment #196
alexpottThis issue still has the tag "Needs issue summary update". Also this change will impact themers so we need a change record for them.
According to the comments we need followup for:
As far as I can tell only 2 of them have issues actually created.
I'm conscious that this has been rtbc for a long time so once the change record has been created, the issue summary checked and followups exist this will go in.
Comment #197
alexpottMore followups :)
Comment #198
LewisNymanComment #199
mortendk CreditAttribution: mortendk commentedAdded followups:
171: nodeteaser classes #2277905: node teaser title classname
195: render cache: #2277909: node render cache
172: meta bartik, stark & drupalbase #2277913: meta: the role of stark & bartik in Drupal8 twig (also takes on the cleaner stark, bartik2 etc)
did i miss anyone of em ?
Comment #200
alexpott172: 2004252-node-171.patch queued for re-testing.
Comment #202
sarahjean CreditAttribution: sarahjean commentedWorking on change record at the DrupalCon contrib sprint
Comment #203
mortendk CreditAttribution: mortendk commentedim in the sprint room come hunt me down if you need an overview of what actually happend
Comment #204
seantwalshStarting the change record while sarahjean works on the re-roll.
Comment #205
sarahjean CreditAttribution: sarahjean commentedReroll of the patch from #172
Comment #206
sarahjean CreditAttribution: sarahjean commentedFinished up change record and patch reroll.
Comment #207
mikl CreditAttribution: mikl commentedGreat work, looked it over, this will make it much cleaner to work with :)
Comment #208
alexpottCommitted b001084 and pushed to 8.x. Thanks!
Comment #209
camoa CreditAttribution: camoa commentedYAYYY!!! Y'all make it happen!!!
Comment #210
yched CreditAttribution: yched commentedSo this did the following in node.html.twig :
#2217731: Move field classes out of preprocess and into templates is about doing the exact opposite in field.html.twig :
Can someone from this issue here chime in over there about the global strategy for "attributes & classes in templates" ?
Comment #211
mortendk CreditAttribution: mortendk commented@yched there has been a lot of confusion about this & frankly its because we didnt have an agreed upon strategy
Theres a followup issue right now on the node.html.twig file that is trying to clean this up.
At drupalcon in austin we finally got a plan for the whole class mess:
global stategy is that all class generating will be moved out of preprocess and into the templates, so the themes dont have to work around preprocess & all that jazz
joel pittet is working on a POC that can make it happen, theres a first step for this here : #2254153: Move node classes out of preprocess and into templates
Comment #212
yched CreditAttribution: yched commented@mortendk: Makes sense. Thanks for the update :-)