- Issue created by @mherchel
- π«π·France pdureau Paris
Currently with Javier. I will review this before any merge.
- πΊπΈUnited States caligan
I've started roughing this out using card-grid and accordion as a pattern.
This is ready for review now, in order to test it you can do the following:
1- copy core/themes/olivero/templates/layout/page.html.twig to wthemes/contrib/drupal_cms_olivero/templates/layout
2- embed the component on page.html.twig:{% embed 'drupal_cms_olivero:statistics-group' with { title: '3 Statistics', description: 'Microcontent description - I think this new service can be a good alternative in this case. I responded with comments for the question regarding the Secret Santa Party.', cta_url: '#', cta_text: 'Optional CTA' } only %} {% block statistics %} {% embed 'drupal_cms_olivero:statistics-item' with { eyebrow: 'EYEBROW TEXT HERE', percentage: 'XXXX%', description: 'Statistic description - I think this new service can be a good alternative in this case.' } only %} {% endembed %} {% embed 'drupal_cms_olivero:statistics-item' with { eyebrow: 'EYEBROW TEXT HERE', percentage: 'XXXX%', description: 'Statistic description - I think this new service can be a good alternative in this case.' } only %} {% endembed %} {% embed 'drupal_cms_olivero:statistics-item' with { eyebrow: 'EYEBROW TEXT HERE', percentage: 'XXXX%', description: 'Statistic description - I think this new service can be a good alternative in this case.' } only %} {% endembed %} {% endblock %} {% endembed %}
See attached screenshot, make sure to test with three, two and one statistic.
- πΊπΈUnited States mherchel Gainesville, FL, US
I gave a quick look at the markup and CSS and added a couple suggestions. Everything is looking great at first glance although I haven't yet tested it (will probably do so next week).
Thanks for working on this!
I'm also crediting @caligan here, since she worked on it at the DrupalCon Atlanta sprints (although it looks like work was duplicated with @javi-er, which happens).
- πΊπΈUnited States caligan
I ran across discussion of the use of 'eyebrow' in the cards component, suggesting 'title' or 'pretitle' instead: https://www.drupal.org/project/drupal_cms/issues/3497385#comment-15934581 β¨ Implement designs for Cards component within Olivero for XB Active
@caligan sorry about that, someone pointed me to this ticket and since it wasn't assigned to anybody I worked on it.
@mherchel I adjusted the details from your feedback, thanks! let me know if you notice anything else.
Regarding the 'aria-labelledby' attribute, I added it to the section, but since headings for sections and articles are required, it shouldn't be necessary to have it, I've done some tests with 'article' elements and headings without it and they are described correctly by accessibility tools (unless it's nested under several elements or inside an anchor element), however it doesn't hurt to include it.
The term 'pretitle' seems appropriate as well for describing the short text above the statistic, naming things is hard. However I remember having this same discussion internally and 'eyebrow' is an old term, widely used in print media for describing these elements too. Let me know what do you prefer and I'll be happy to change it.
By the way, @caligan feel free to push changes to this MR if you feel too, I don't mind working on this collaboratively.
- πΊπΈUnited States caligan
@javi-er - Not a problem at all, I hadn't convinced myself I was up to it yet and should've commented sooner. Looking good!
Regarding names, though, maybe 'percentage' should be 'value'? Since it's not always a percentage, it implies a constraint that doesn't exist, and could give the incorrect impression it'll append % automatically.
- π«π·France pdureau Paris
Hi Javier,
It would have been better to use https://www.drupal.org/project/sdc_devel β because it would have caught half of my feedbacks.
statistics-group
component: Twig templateYou didn't use the
attributes
variable. A default attributes object is always injected in template and expected by many Drupal API (for example: adding a10n attributes, i18n attributes, compatibility with|set_attribute()
and|add_class()
filters...)It will also allow you do manage
aria-labelledby
in a more readable way:{% set attributes = statistics_group_id ? attributes.setAttribute("aria-labelledby", statistics_group_id) : attributes %}
Avoid
clean_unique_id
because this function call the application state through Html::getUniqueId(). A component must stay stateless.So, instead of:
{% set statistics_group_id = title ? ('statistics-group-title'|clean_unique_id) : null %}
You can do something like that:
{% set statistics_group_id = statistics_group_id|default('statistics-group-title--' ~ random()) %}
statistics-group
component: YAML definitionYou used a reference to a definitions provided by Experience Builder module, so this may break if
experience_builder
(which is still in an early phase and is expected to change a lot before 1.0.0) is not activated:$ref: json-schema-definitions://experience_builder.module/textarea
It is better to directly write the expected schema:
type: "string", pattern: "(.|\\r?\\n)*"
cta_url
must be an URL instead of a just string. Proposal:cta_url: type: string format: iri-reference
description
may be better as a slot, to be able to inject any renderable in the DIV:{% if description %} <div class="statistics-group__description">{{ description }}</div> {% endif %}
(It is not mandatory to use Twig blocks for slots, so you can keep the template like that)
statistics-item
component: Twig templateSame feedback for the
attributes
variable.statistics-item
component: YAML definitionSame feedback for
$ref: json-schema-definitions://experience_builder.module/textarea
. Also, this reference is particularly problematic because named as a form widget instead of a data structure (as shared with XB team last week). It is better to avoid it.Same feedback for
description
which may be better as a slot.Other
drupal_cms_olivero
componentsBecause the development is happening outside of Drupal Core, those Olivero component went a bit under the radar and could be fixed and improved:
- Same issues about
$refs
to XB - Same overuses of props when slots will be more suitable (examples:
content
in Paragraph) - Testimonial use the internal
componentMetadata.path
instead of the newicon()
function. - Testimonial use an undefined
source
variable - Search Narrow and Search Wide use undefined
title_attributes
,label
,content_attributes
andcontent
variables - Search Wide embed a SVG with the
include()
function instead of the expectedsource()
Twig function (or, better, the icon() fucntion from the new Icon API) - ...
I will create a dedicated ticket for them.
- Same issues about
Thanks for the feedback @pdureau ! yes, you mentioned
sdc_devel
to me but I completely forgot about it when I got to work on the component, instead I used as example the upcoming accordion component, which it's also facing similar issues now that I look at it (#3508546). I rushed it up to have it done before the contribution sprint ended, but I can iterate on this as many times as necessary until it's good.When I try to see the
sdc_devel
report I'm getting this error, searching the issue queue I found this one π Fix issue LogicException: Attribute "name" does not exist for Node "Twig\Node\Expression\GetAttrExpression". Active that's similar and should be fixed, I'll post an issue on the module later regarding this.
LogicException: Attribute "value" does not exist for Node "Twig\Node\Expression\Binary\ConcatBinary". in Twig\Node\Node->getAttribute() (line 158 of /var/www/html/vendor/twig/twig/src/Node/Node.php).
It makes sense to use the
attributes
variable, I was wondering about this when implementing it, also I like that the objective is to keep components free of Drupal specific functions, it should make them more compatible with external systems.There is a reference to a definition provided by Experience Builder module, so this may break if the module (which is still in an early phase) is not activated
It actually breaks if the module is not enabled, I had to enable it in order for my component to render. Looking at the other existing components, most of them use these XB specific schemas, if the objective is for these components not to require XB, it should be fixed in almost all the other components under
drupal_cms_olivero/components
as well. I changed this one to use the regex you suggest.description may be better as a slot, to be able to inject any renderable in the DIV
Agree, it's very likely that this text comes from CKE in the future, I changed it. I switched t a block for consistency, but I can use a variable instead if prefered.
Let me know if you have more feedback, this also needs a CSS review and a functional review. I tested with every combination I could try to make sure that it works well under most use cases.
- π«π·France pdureau Paris
It makes sense to use the
attributes
variable, I was wondering about this when implementing it, also I like that the objective is to keep components free of Drupal specific functions, it should make them more compatible with external systems.I am totally aware of that. In a perfect world, it would be better to not have such Attribute PHP object in our template, because all PHP objects must be avoided in template, and because it is better to stick as much as possible with vanilla Twig. But we don't have a replacement for this yet.
Today, among all the stuff added by Drupal to Twig, I see only 5 additions we can use in component templates:
- the
PrintNode
visitor to send all printed variable to the renderer service. That's the core of Drupal Twig rendering and must be kept. - this
Attribute
object and the relatedcreate_attribute()
function, but I hope this issue will allow us to get rid of it one of those days: β¨ HTML attributes as Twig mappings instead of PHP obejcts Active add_class()
&set_attribute()
filters, they make sense with the SDC slots and they follow Twig philosophy of altering existing data.- the new
icon()
function for the Icon API. It follows Twig philosophy of using function to generate printable data - the translations helpers (
t
function,trans
tag). Important feature for a CMS. Why not proposing this to upstream Twig?
Everything else (
clean_id()
,attach_library()
,link()
,|add_suggestion()
...) is avoidable.Agree, it's very likely that this text comes from CKE in the future, I changed it. I switched t a block for consistency, but I can use a variable instead if prefered.
I prefer to simply print my slots, but blocks are OK too. It is a matter of personal taste (except a few situations π± Clarify SDC documentation by toning down Twig blocks promotion Active where blocks may be problematic).
Let me know if you have more feedback,
I will have a look this week.
- the
- πΊπΈUnited States bernardm28 Tennessee
I agree with most of Pier's additions, but one thing. clean_unique_id provides a more semantic version than random(), and it's a better experience for the end user.
I would argue that even if it violates the component state, there's no better alternative. What would be a downside of having it in practice? Would it not get cached as good, or is it more of the principle behind it?