Create "statistics" component for Olivero in Drupal CMS

Currently with Javier. I will review this before any merge.

I've started roughing this out using card-grid and accordion as a pattern.

Edit: Sorry, I see this is in progress and will hold.

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.

i take it

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

@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.

@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.

Added parent issue

@caligan yes I agree, "value" makes much more sense, updated.

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 template

The attributes variable is not used. 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 outside the HTML element markup (so in a more readable way, IMHO):

{% 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 definition

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:

      $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 template

Same feedback for the attributes variable.

statistics-item component: YAML definition

Same feedback for $ref: json-schema-definitions://experience_builder.module/textarea. Moreover, as shared with XB team last week, this reference has the name of a form widget instead of a data structure.

Same feedback for description which may be better as a slot.

Other drupal_cms_olivero components

Because 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 new icon() function.
• Testimonial use an undefined source variable
• Search Narrow and Search Wide use undefined title_attributes, label, content_attributes and content variables
• Search Wide embed a SVG with the include() function instead of the expected source()Twig function (or, better, the icon() function from the new Icon API)
• ...

I will create a dedicated ticket for them.

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 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.

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 related create_attribute() function, but I hope this issue will allow us to get rid of it one of those days: #3457874: HTML attributes as Twig mappings instead of PHP objects
• 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 where blocks may be problematic).

Let me know if you have more feedback,

I will have a look this week.

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?

Also, thanks for dropping so many insights.

@bernardm28 I like clean_unique_id as well since it adds an incremental value if necessary instead of a random number, but in terms of experience for the actual user it doesn't change anything since screen readers don't actually announce the id value, neither is visible in any way in the browser, maybe in the URL if you link to it.

I'm really, really sorry to do this, but given that we are shifting to site templates in 2.0, we don't plan to add new features to the 1.x branch and Olivero in particular. Everyone who worked on this should still get credit for their contributions, which we really appreciate!