The way that we're implementing the layout CSS class is a bit confusing. We can make it more clear.

1) We can remove the node--article--full.html.twig file, and add the layout--content-narrow class via preprocess.
2) We need to remove the layout variable from node--teaser.html.twig.

Original summary

Here is the content of node--article--full.html.twig;

{% include '@olivero/node.html.twig' with
  {
    'layout': 'content-narrow',
  }
%}

The layout variable is concatenated by after in node.html.twig like this:
{% set layout = layout ? 'layout--' ~ layout|clean_class %}

Question 1: Why over-complicate with the concatenation? The themer needs to know that he/she have to use part of the class name (without the starting layout--)... a more simple way would be 'layout': 'layout--content-narrow',

Question 2: Why using an include if it is used only once? More over, in the case of a subtheme @olivero could lead to an issue for someone who have edited node.html.twig [I am completely open to hear a good reason for that...]

Question 3: why using the layout variable in node--teaser.html.twig as I don't see where it is populated?... But maybe I missed something.

CommentFileSizeAuthor
#15 3142857-15.patch1.35 KBmherchel
#12 3142857-12.patch1.17 KBmherchel
#10 3142857-9.patch1.18 KBsulfikar_s
#10 after-patch-apply.png23.34 KBsulfikar_s
#8 3142857-8.patch1.04 KBAbhijith S
#7 interdiff_6_7.txt839 bytesanmolgoyal74
#7 3142857-7.patch1.36 KBanmolgoyal74
#6 3142857-6.patch1.05 KBkostyashupenko
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuneBL created an issue. See original summary.

DuneBL’s picture

DuneBL
Primary language French
CreditAttribution: DuneBL commented
Issue summary: View changes
steinmb’s picture

steinmb CreditAttribution: steinmb as a volunteer commented
Version: 8.x-1.0-alpha1 » 8.x-1.x-dev

I had a look at the code but I found it a little confusing too. Perhaps @mherchel know? Introduces in #3118071: Theme Content/Article Listing - Node Teaser

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented

The article node content type has a narrower layout than other content types (see designs at https://www.figma.com/file/x5zBLbvoW1jsvyAOt4Gp9I/Olivero-Theme---Public...).

1) I personally don't think it's too complicated, but I can see your point.
2) This is a good point. We could easily add this class via preprocess instead.
3) I'm not sure this variable is being used. This may need to be removed.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Title: layout class is not populated in some template + why using include » Refactor Olivero's usage of layout CSS class in node--article--full, and node--teaser
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
Category: Bug report » Task
Issue summary: View changes
Priority: Normal » Minor
kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko commented
Status: Active » Needs review
FileSize
3142857-6.patch1.05 KB

node-teaser.html.twig was updated here already

so i have fixed only first point

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
FileSize
3142857-7.patch1.36 KB
interdiff_6_7.txt839 bytes

Remove the additonal space and add the comment.

Abhijith S’s picture

Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commented
FileSize
3142857-8.patch1.04 KB

Patch #6 is working fine.Since #7 is failed ,I've removed additional white-space from #6 . Please check it

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch doesn't apply

sulfikar_s’s picture

sulfikar_s CreditAttribution: sulfikar_s at Zyxware Technologies commented
FileSize
after-patch-apply.png23.34 KB
3142857-9.patch1.18 KB

Hello, the patch in #8 doesn't apply as said in #9.

after-patch-apply.png

I have re-rolled the patch in #8.

Please review.

Edit: Sorry for the patch number, I've mistakenly added it as '9'. It is actually '10'

sulfikar_s’s picture

sulfikar_s CreditAttribution: sulfikar_s at Zyxware Technologies commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Issue tags: -Needs reroll
FileSize
3142857-12.patch1.17 KB 
  1. +++ b/core/themes/olivero/olivero.theme
@@ -100,6 +100,10 @@ function olivero_preprocess_node(&$variables) {
+  if ($variables['node']->bundle() == 'article' && $variables['view_mode'] == 'full') {

    We want to use strict type checking here (=== instead of ==) 

  2. +++ b/core/themes/olivero/templates/content/node--article--full.html.twig
@@ -4,8 +4,3 @@
-%}

    You're removing the content, but leaving the template. This means that the article will not show at all. We need to remove the entire template file.

Attaching updated patch.

proeung’s picture

proeung
she/her
CreditAttribution: proeung as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/themes/olivero/olivero.theme
@@ -100,6 +100,10 @@ function olivero_preprocess_node(&$variables) {
+    $variables['layout'] = 'content-narrow';
+  }

+++ /dev/null
@@ -1,11 +0,0 @@
 

A bit of nitpicking, but I think it'll be helpful to include a comment in this preprocess function that lets folks know that this string is being used in the node template where we're defining the layout class.

'layout--' ~ layout|clean_class

proeung’s picture

proeung
she/her
CreditAttribution: proeung as a volunteer commented
Title: Refactor Olivero's usage of layout CSS class in node--article--full, and node--teaser » Refactor Olivero's usage of layout CSS class in node--article--full

Adjust the title of this issue to include just the node--article--full node template.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel at Lullabot commented
Version: 9.1.x-dev » 9.2.x-dev
Status: Needs work » Needs review
FileSize
3142857-15.patch1.35 KB

Good point. Updated patch attached!

proeung’s picture

proeung
she/her
CreditAttribution: proeung as a volunteer commented
Status: Needs review » Reviewed & tested by the community

@mherchel Looks great! Thanks for adding this comment!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 9.1.x as this is for an experimental theme.

Committed and pushed a997779d33 to 9.2.x and 9bc878583d to 9.1.x. Thanks!

  • alexpott committed a997779 on 9.2.x 
    Issue #3142857 by mherchel, sulfikar_s, anmolgoyal74, kostyashupenko,...

  • alexpott committed 9bc8785 on 9.1.x 
    Issue #3142857 by mherchel, sulfikar_s, anmolgoyal74, kostyashupenko,...

Status: Fixed » Closed (fixed)

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