Convert Olivero's teaser into a single directory component

After a few glitches, it's 99% ready. I will work on the cleanup on a follow-up commit.

Just left some comments in the MR. Looking good. Thanks!

I added the requested changes.

Threads look to be resolved but postponed until SDC is marked stable,

Hi all!

I think it's time to continue working here!

I believe that SDC first of all gives us the opportunity to clean up the cascades and make everything more stable. Therefore, in this MR, in addition to the immediate changes in the teaser, I added a few more changes.

- Removed styles from the field styles file

- Rewrote the view style to use the grid. This is much better than constantly adjusting margins and much more correct from a BEM point of view. A small regression of a few pixels on mobile devices, but the code has become much more predictable.

https://en.bem.info/methodology/quick-start/#block

The block shouldn't influence its environment, meaning you shouldn't set the external geometry (margin) or positioning for the block.

- Removed the picture template. It only existed to add one class

- core/themes/olivero/css/components/node-teaser.pcss.css still contains styles, but we will remove them in future componentization work.

Please review!

Before

After

Took a screenshot with the MR not applied yet
Applied the MR
Took another screenshot and can see nothing has changed

Know it's not the same but recently got into ui_patterns so able to review that and the slot options do make sense.

why the empty js file ?

from what I understood if you want to have a render array it should be in a slot, not a prop so content should be a slot no?

smustgrave → changed the visibility of the branch 3365367-convert-oliveros-teaser to hidden.

Ah now I see what you meant, wrong MR. I should of hidden that one.

nod_ → closed merge request !7250

Committed abf79d7 and pushed to 11.x. Thanks!

I am late to the subject because the change has been merged today, but I am not comfortable with this task.

We have 2 kind of templates to consider for conversion to SDC:

• status-messages.html.twig, breadcrumb.html.twig, table.html.twig, image.html.twig, username.html.twig, pager.html.twig, menu.html.twig... which are UI components and will shine as SDC componenst
• node.html.twig, user.html.twig, field.html.twig, block.html.twig, views.html.twig... which are directly related to Drupal data structure and acts as wrappers or proxy layers

Do we really want to convert to SDC the second kind?

Looking at this MR, it seems we rather not:

• The template has parts unexpected in a component template : related to context ({% if label and not page %}) and data structure (content|without('field_image', 'links'))
• This component doesn't bring anything valuable in an UI point of view, and is hardly reusable outside of a node teaser context
• The template use BEM notation but the teaser block class is missing.

I would advice to revert this change.

Maybe the future of node.html.twig, user.html.twig, field.html.twig, block.html.twig, views.html.twig... is to be ignored first, and to be removed once SDC will be the main API to build template-based renderables.

Thank you for review.

We can update it with slots. And put node teaser logic to its own template.

I see that this component can be also used in comments. They have mostly same look.

Could you please create same follow up ticket a you did for umami? We arr still researching how to use SDC correct

Hi Ivan,

I will create the follow up ticket about the little fixes i am proposing.

But the main position of my review is: Do we really want this component? And this kind of components in general?

Do we really want this component? And this kind of components in general?

I believe that this type of component, just like the umami card, is the fundamental building block for any website.

Yes, in Olivero it is used once (for now I hope). But developers should see a good example of a good component in Drupal code

Do we really want to convert to SDC the second kind? (related to Drupal data structure and acts as wrappers or proxy layers)

Evil designers often draw components instead of drawing the Drupal structure (joke)

I believe that components should not be associated with the Drupal structure. So from my point of view Yes. We need such components. Even if we don't have good way to implement them except twig (for now I hope)

Comment 20 -- https://www.drupal.org/project/drupal/issues/3365367#comment-15557100 📌 Convert Olivero's teaser into a single directory component Postponed says that this was merged to 11.x but the issue is now assigned to 10.3.

Is the intent to cherry-pick that commit to 10.3, or is the change accidental?

See #18

I moved my concern to a dedicated issue: 🌱 Compatibility of SDC use with display/experience builders Active

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

Sorry very late here but I think we should consider reverting this.

After this change I do not understand what's going on in core/themes/olivero/templates/content/node--teaser.html.twig.

{% embed "olivero:teaser" with {
  attributes: attributes.addClass(classes),
  author_attributes,
  author_name,
  content,
  date,
  display_submitted,
  label,
  metadata,
  title_attributes,
  title_prefix,
  title_suffix,
  read_more,
  url,
  } only %}
  {% block prefix %}
    {{ title_prefix }}
    {{ title_suffix }}
  {% endblock %}
  {% block meta %}
    {% if display_submitted %}
      <div class="node__meta">
        <span{{ author_attributes }}>
          {{ 'By'|t }} {% apply spaceless %}<span class="node__author">{{ author_name }}</span>{% endapply %}, {{ date }}
        </span>
        {{ metadata }}
      </div>
    {% endif %}
  {% endblock %}
  {% block image %}{{~ content.field_image ~}}{% endblock %}
  {% block title %}
    {% if label and not page %}
      <h2{{ title_attributes.addClass('node__title', 'teaser__title') }}>
        <a href="{{ url }}" rel="bookmark">{{ label }}</a>
      </h2>
    {% endif %}
  {% endblock %}
  {% block content %}
    {{ content|without('field_image', 'links') }}
  {% endblock %}
{% endembed %}

vs. the component itself:

<article{{ attributes.addClass('teaser') }}>
  <header>
    {% block prefix %}{% endblock %}
    <div class="teaser__meta">
      {% block meta %}{% endblock %}
    </div>
    <div class="teaser__top">
      <div class="teaser__image">{% block image %}{% endblock %}</div>
      {% block title %}{% endblock %}
    </div>
  </header>
  <div class="teaser__content">
    {% block content %}{% endblock %}
  </div>
</article>

Why do we have markup for the image in the sdc template, then pull that back out into the node template?

The Olivero template already breaks some capabilities of manage display, and this just looks like it's getting even further away from that.

There are plenty of templates in core which don't have this problem (I arrived here looking for an example to review 📌 Convert Olivero's pager to use single directory components Needs work against), so why not do those first per #21?

Why do we have markup for the image in the sdc template, then pull that back out into the node template?

it is optional slot. it can be presented or not.

@catch please take a look at #15 #16

https://www.drupal.org/project/drupal/issues/3444525#comment-15689724 🌱 Compatibility of SDC use with display/experience builders Active

Let's discuss first at least.