Olivero's meta component should be using SDC. This is referenced in both the node teaser and node templates

During the component development process, we divide the areas of responsibility of CSS classes according to:
https://en.bem.info/methodology/css/#external-geometry-and-positioning

Thus, the node__meta class is responsible for external geometry, and the component itself takes all internal properties

Issue fork drupal-3365377

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mherchel created an issue. See original summary.

megclaypool’s picture

Assigned: Unassigned » megclaypool

Gauravvvv made their first commit to this issue’s fork.

gauravvvv’s picture

Status: Active » Needs review
smustgrave’s picture

Assigned: megclaypool » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

FYI should avoid assigning tickets to yourself unless you're a maintainer
https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

Should just leave a comment you'll be working on it

Seems this MR is causing failures.

gauravvvv’s picture

Status: Needs work » Needs review
mherchel’s picture

Status: Needs review » Needs work

This is failing coding standards. There's also a lot of unrelated whitespace changes that should not be made here.

dieterholvoet’s picture

Component: theme system » Olivero theme

finnsky made their first commit to this issue’s fork.

finnsky’s picture

Status: Needs work » Needs review

Hello all!

Pushed new MR with different approach.

We write components to make them reusable not only in node or node--teaser.
It should be encapsulated component which can appear in paragraphs, cards wherever.

It keeps all inner css inside. Its external geometry still controlled by `.node__meta` css class.
https://en.bem.info/methodology/css/#external-geometry-and-positioning

Please review!
Thanks

smustgrave’s picture

Status: Needs review » Needs work

To see if this passes tests you can include sdc as an olivero dependency.

Just FYI until SDC is marked stable it can't be merged to olivero so these tickets for olivero and sdc will ultimately be postponed until that. But it's good to have them ready :)

finnsky’s picture

@smustgrave
Yep. I know. Just want to to understand how it should/may look like in future.
And we still may review it. At least in terms of independency of component.

manojkumar_97 made their first commit to this issue’s fork.

finnsky’s picture

@manojkumar_97 why you removed component? :)

finnsky’s picture

Rebased. but work still needed here.

finnsky’s picture

Status: Needs work » Needs review

Some random test failures happens.

smustgrave changed the visibility of the branch 3365377-olivero-convert-node to hidden.

smustgrave’s picture

Status: Needs review » Needs work

Have not yet reviewed but see latest MR has a number of test failures.

eojthebrave made their first commit to this issue’s fork.

eojthebrave’s picture

The test failures are due to this error:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [avatar] NULL value found, but an object is required in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

Caused by the fact that the `author_picture` variable in the node template file is sometimes set, and sometimes not. For example if the feature is disabled in the theme, or if the user has no picture. This results in the `avatar` prop getting set to NULL, and the above error. The fix appears to be to not pass an `avatar` prop if we have no value for author_picture. I've added a commit that does that, though it's possible there are better approaches I couldn't figure out a way using Twig to conditionally set just one key in an object.

Let's see if there are any more failures after fixing that.

Mithun S made their first commit to this issue’s fork.

mithun s’s picture

Status: Needs work » Needs review

Rebased the branch with latest changes from the target branch. The MR seems to be passing the test cases.
Please review the MR. Changing the status to Needs Review.
Thank you!

ahsannazir made their first commit to this issue’s fork.

ahsannazir’s picture

StatusFileSize
new13.69 KB

The SDC component for meta is working as expected. Attaching screenshot for reference.

smustgrave’s picture

Status: Needs review » Needs work

Left some comments in the MR.

finnsky’s picture

Gonna check

finnsky’s picture

Status: Needs work » Needs review
ahsannazir’s picture

StatusFileSize
new9.55 KB

The Meta component is looking as expected.

smustgrave’s picture

Tried revisiting this one but still have concerns about the missing/changing class names.

shweta__sharma’s picture

smustgrave’s picture

Status: Needs review » Needs work

@shweta__sharma if tagging for issue summary update mind leaving a comment also.

finnsky’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -SDC Sprint DrupalConNA 2023, -Needs issue summary update
smustgrave’s picture

Status: Needs review » Needs work

I think this needs a rebase as nightwatch keeps failing (re-ran twice)

finnsky’s picture

Status: Needs work » Needs review

rebased

finnsky changed the visibility of the branch 3365377-metadata to hidden.

finnsky changed the visibility of the branch 3365377-metadata to active.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Rebase seems good. I did a standard install on 11.x and verified the meta data appears unchanged.

Still a little iffy about the class changes but won't hold the issue up over it.

nod_’s picture

Assigned: Unassigned » pdureau
Status: Reviewed & tested by the community » Needs review

Would like pdureau to check this

pdureau changed the visibility of the branch 3365377-metadata to hidden.

pdureau’s picture

Assigned: pdureau » Unassigned
Status: Needs review » Needs work

Mandatory fixes

Slots are not typed

So let's remove the type: object here:

  metadata:
    title: Metadata
    description: Additional metadata
    type: object

Consistent slot notation

The 2 slots have different notations, one with a block and a print node, one with only a block:

{% block avatar %}{{ avatar }}{% endblock %}
{% block metadata %}{% endblock %}

It is better to stay consistent. My personal preference would be no blocks (see: #3484727: Complete and clarify SDC documentation), but if there is a block I would advise to add also the print node.

A summary (without the only and with_context keywords, for clarity):

Slot in the component template When called from a Drupal template
{% block label %}{% endblock %} {% embed(my_component) %}{% block label %}{{ label }}{% endblock %}{% endembed %}
{{ label }} {{ include(my_component, {label: label}) }}
{% block label %}{{ label }}{% endblock %} Heavy, but compatible with both include and embed

Deprecated Twig filter: `spaceless`

The spaceless filter is deprecated as of Twig 3.12. While not a full replacement, you can check the whitespace control features.

Other feedbacks

As always, the attributes prop declaration is not necessary because automatically added to all components, but it doesn't hurt to add it so keep if you want:

    attributes:
      title: Attributes
      description: Meta attributes.
      type: Drupal\Core\Template\Attribute

In attributes and author_attributes, using a PHP Namespace as a prop type is not JSON schema valid and is skipping the SDC validator. However, describing an Attribute object with JSON Schema is complex without a reference resolver, so we can let it like that for now.

finnsky changed the visibility of the branch 3365377-metadata to active.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.