Needs work
Project:
Drupal core
Version:
main
Component:
Olivero theme
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
7 Jun 2023 at 15:18 UTC
Updated:
7 Feb 2025 at 08:11 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #2
megclaypool commentedComment #5
gauravvvv commentedComment #6
smustgrave commentedFYI 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.
Comment #7
gauravvvv commentedComment #8
mherchelThis is failing coding standards. There's also a lot of unrelated whitespace changes that should not be made here.
Comment #9
dieterholvoet commentedComment #12
finnsky commentedHello 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
Comment #13
smustgrave commentedTo 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 :)
Comment #14
finnsky commented@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.
Comment #16
finnsky commented@manojkumar_97 why you removed component? :)
Comment #17
finnsky commentedRebased. but work still needed here.
Comment #18
finnsky commentedSome random test failures happens.
Comment #20
smustgrave commentedHave not yet reviewed but see latest MR has a number of test failures.
Comment #22
eojthebraveThe 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.
Comment #24
mithun sRebased 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!
Comment #26
ahsannazir commentedThe SDC component for meta is working as expected. Attaching screenshot for reference.
Comment #27
smustgrave commentedLeft some comments in the MR.
Comment #28
finnsky commentedGonna check
Comment #29
finnsky commentedComment #30
ahsannazir commentedThe Meta component is looking as expected.
Comment #31
smustgrave commentedTried revisiting this one but still have concerns about the missing/changing class names.
Comment #32
shweta__sharma commentedComment #33
smustgrave commented@shweta__sharma if tagging for issue summary update mind leaving a comment also.
Comment #34
finnsky commentedComment #35
smustgrave commentedI think this needs a rebase as nightwatch keeps failing (re-ran twice)
Comment #36
finnsky commentedrebased
Comment #39
smustgrave commentedRebase 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.
Comment #40
nod_Would like pdureau to check this
Comment #42
pdureau commentedMandatory fixes
Slots are not typed
So let's remove the
type: objecthere:Consistent slot notation
The 2 slots have different notations, one with a block and a print node, one with only a block:
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
blockI would advise to add also the print node.A summary (without the
onlyandwith_contextkeywords, for clarity):{% block label %}{% endblock %}{% embed(my_component) %}{% block label %}{{ label }}{% endblock %}{% endembed %}{{ label }}{{ include(my_component, {label: label}) }}{% block label %}{{ label }}{% endblock %}includeandembedDeprecated 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
attributesprop declaration is not necessary because automatically added to all components, but it doesn't hurt to add it so keep if you want:In
attributesandauthor_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.