Implement designs for Feature component within Olivero for XB

Escalating to critical because @ckrina told me it is a must-have issue for the Experience Builder preview to be viable. I am not calling it a stable blocker, though, since the XB preview is not itself a stable blocker.

Thank you @ckrina for Figma links, @balintbrews for the eval command, @phenaproxima for implementing a bridge between XB and the Drupal CMS.

I was able to finalise the Front-end for the Feature component (screenshots provided in the files section).

DONE:
- Desktop
- Mobile

TODO:
- Need to add tags to props, currently they are hardcoded
- Need to figure out how to add a condition on the image (add .feature--has-no-image to the parent div when image is not provided)

- Need to figure out how to add a condition on the image (add .feature--has-no-image to the parent div when image is not provided)

You may be able to use :has() inside :not() instead of adding a class.

div:not(:has(img)) {
  background: blue;
}

No slots in this component? Why the slots get so little love in the SDC community those days? 🙃

A few feedbacks.

image prop must be a slot

The $ref of this prop:

    image:
      title: 'Featured image'
      $ref: json-schema-definitions://experience_builder.module/image
      type: object
      examples:
        - src: https://placehold.co/600x450.png
          alt: 'Boring placeholder'
          width: 600
          height: 400

has 2 issues which can be resolved by switching to a slot:

• an unnecessary dependency to experience builder. There is nothing wrong referencing a $def from experience_builder.module per se, but in this specific case it is better to not bend the component definition to please the display builder, whatever it is.
• a rigid structure to represent an object.

I have already warned XB team about this problematic $ref : https://www.drupal.org/project/experience_builder/issues/3468944#comment...

Today, the MR template looks like that:

  <img alt="Placeholder Image" class="my-section__img" width="600" height="450" src="data:image/jpeg;base64...">

But I guess it will looks like that later:

<img class="my-section__img" src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}"/>

This rigid structure doesn't fit with real life usage. What will happen if some props are missing or empty? What about other classes? What about other attributes? cross origin rules? lazy loading? responsive sources?

A simple image slot with {{ image|add_class( "my-section__img") }} will fix all those issues.

Experience Builder is an exciting but early product, with some youthful quirks. Let's not impact the drupal_cms_olivero integrity with them. Lets' build the best components first and wait the display builders to get better.

cta as a slot to avoid prop drilling

We have this in the definition:

    cta:
      type: string
      title: CTA text
    ctahref:
      type: string
      format: uri
      title: CTA link

And this in the template:

<div class="featured__cta-wrapper">
   <a class="button--primary" href="{{ ctahref }}">{{ cta }}</a>
</div>

button--primary is clearly its own component, distinct from feature, so we have those 2 issues:

• the button component markup is duplicated in the feature component template, which will cause maintenance issues when the former will evolve
• the feature component is too busy (2 unexpected props!) because of the "prop drilling" (defining props in a component to pass them in a child component)

We need a cta slot, where it will be possible to inject a button component (or anything else 😉).

Missing attributes object

We currently have this in the MR:

<div{{ create_attribute({'class': classes}) }}>

2 issues:

• classes is missing from the component definition
• it is nice to allow the user to add HTML classes, but not enough

So, why not leveraging the Attribute object called attributes, injected in all component template by SDC, instead?

attributes is great because it is a standardized powerful API endpoint for all components:

• SEO modules can insert tagging attributes through it
• Accessibility modules can alter/insert some attributes values (aria-*, role, alt...) through it
• Translation modules can add the lang attribute when needed
• Styles utilities modules (like UI Styles, Style Options...) can insert helpers classes through it
• Hypermedia modules (HTMX, Hotwire/Turbo...) can insert triggers and behaviours attributes through it
• Drupal Core's |add_class() and |set_attribute() Twig filters rely on it
• And, of course, display builders (like Experience Builder) can insert annotation through it

Happy to discuss further if you find those feedbacks useful.

With gratitude and appreciation for the feedback, I would like to point out something here:

This does not need to be perfect, or feature-complete, or even close to it. This is strictly meant for a one-off, single-page, read-only demo of Experience Builder, which is under heavy development, and we are not (for now) going to allow people to build real production pages with these components. We have a lot of leeway to change them in future releases of Drupal CMS, although we will try to maintain backwards compatibility in our Olivero subtheme.

Having said that, If changing the image to a slot is easy to do (I am not a front-end developer so I have no idea) and costs us nothing (or very little) in time and effort, great! I leave it up to @boulaffasae to decide whether or not to do that. But this needs to get done, and it needs to get done this week, so let's prioritize getting it done even if has rough edges.

I think those are excellent points about avoiding prop drilling, @pdureau. One concern I have is that XB is not yet able to limit which components can be placed inside a slot, nor can it limit how many components you can place.

Because of that, I'm leaning towards advocating for more props for the time being. The goal with these first SDCs is to give an early preview of features that work in XB. I'm worried that going too heavy on slots would do more to highlight what doesn't work yet.

Hi everyone,

I updated the component.yml file

** Added a new Boolean (with_image: Specifies whether to include an image. Set to true to include an image, or false to exclude it.)
** Replaced the Image prop with an Image slot

I hope this saves us time for the demo.

At the same time I would love to share some insights :

1. Slots seem to be limited to only one component, therefore I couldn't use it for Tags and I kept them hardcoded.
2. This component specifically has two variants (with/without image) to help simplify the process, I added the new Boolean (with_image). Without it we will be left with a slot space for image - or a static HTML and we can't remove it (in case it's a slot it will show the slot empty space :/)
3. slots accept only a String value in the examples entry, this prevented me from turning the CTA into a slot because they will need to drop a component there to run the demo :/ (It’s not difficult, yes, but It's not important either at least not for now)

Wow, awesome to be able to start testing this!! And thank you so much @boulaffasae for jumping in :)

After a quick test I removed the tags element altogether, the use case in general of tagging a feature card isn't super clear to me, and we have not included tags in the teaser card in any other context in Drupal CMS. So given this and the potential complexity I think it makes sense to leave it out for now.

Otherwise it works great, the image selection modal is pretty broken but will check the XB queue for that and log it there if it doesn't already exist.

I'll leave it at Needs review in case anyone else has feedback on the implementation but I'm really super excited to see this in action!!!

Also just tweaked some of the text and labels. It wasn't immediately obvious to me that the image was a separate component, so I used the sub-heading to explain this.

And the field descriptions aren't displaying so maybe better to remove them?

Lastly the button was called 'Optional CTA button' but if it's blank, it still outputs a button element so it isn't really optional. I don't mind this though, I updated the label anyway.

I have no objection to merging this as-is, but I'd prefer if the RTBC came from someone else.

Hi @pameeela,

Thank you for your reviews (and your kind words).

@phenaproxima I skipped condition for displaying/hiding props :

Instead of doing

  {% if ctahref %}
    <div class="featured__cta-wrapper">
      <a class="button--primary" href="{{ ctahref }}">{{ cta }}</a>
    </div>
  {% endif %}

I only did

    <div class="featured__cta-wrapper">
      <a class="button--primary" href="{{ ctahref }}">{{ cta }}</a>
    </div>

Just to keep the code aligned with every other XB component template. We will need to apply these adjustments in the near future.

Would we be able to skip the Include image prop if we used a prop for the image instead of the slot? If so, I would strongly recommend doing that.

As I said in #16, I agree with #14 as the long-term direction. However, for these first few components, we would be demonstrating a cleaner user experience with a simple image prop.

Not only would we remove the need for the Include image checkbox as a prop, but we would also make it explicit how an image can be added. Right now, users need to know to place an Image component into the slot. However, they can add other components too, as mentioned in #16.

I feel strongly that at this stage of XB, using the image as a prop would better present the capabilities than implementing it as a slot. XB needs more work on slots to use their full potential. It will be exciting to get there, but for now, my vote is for props when possible.

@balintbrews agree on using a prop for image for the sake of simplicity. This is all new to us and especially since slots can't be limited, this is sure to create more confusion. If this also negates the need for the 'Include image' option, that's great too! But won't it have a placeholder image still so would need to be explicitly disabled?

This does not need to be perfect, or feature-complete, or even close to it. This is strictly meant for a one-off, single-page, read-only demo of Experience Builder, which is under heavy development, and we are not (for now) going to allow people to build real production pages with these components.

Thanks @phenaproxima for sharing with me this context.

Hi @balintbrews, no not really I switched to slots just to make it easier.

The Include image prop is needed to manage the Feature with no Image case.

- Props are already broken (I can neither upload an image through Media Library, nor use image is empty condition)
- Slots can not be hidden :/

This is how I did it for the Card component: 71a0718b.

Let's ignore quirks and bugs of the media library dialog in XB. We'll iron those out, but this should be okay for a preview, I think, and it shouldn't block these components.

Reverting the image slot back to props due to limitations with the current image implementation.
Thank you, @balintbrews, for your suggestion—it makes the process more streamlined.

I need clarification regarding the desktop design—there seems to be a variation in the padding of the card. Could someone confirm which desktop design we should proceed with?

Hi @vasantha deepika,

Content (wrap title, subtitle, and Tags) padding is 45px top/right/bottom/left, it should be centered in the middle if they image size is bigger.

If no image exist, then the padding for the Content again should be top 84, left/right 94, and bottom is 54.

And the latest example, because Image size is less than content, the image get centerd.

I hope this clarify everything for you ^^

@balintbrews Thank you for your feedback; I am updating the MR accordingly.

@boulaffasae Thank you for your clarification!

I have updated MR #372 based on the feedback provided in the comments and changed its status to 'Needs Review' for further evaluation.

This is looking super great! Thank you @boulaffasae and @vasantha deepika.

I talked to @jwitkowski (the designer) and we need some changes:

• Image needs to be 4/3 aspect ratio
• Text needs to be aligned center
• Per https://drupal.slack.com/archives/C072JMEPUS1/p1736350025239129, no way to implement tags in XB. So we'll omit them. Need a followup
• I asked Jen if we should implement a reverse option, and she said yeah. It should be trivial to do

I'm going to work on this now to get it in for 1.0

Working on this.

Style update pushed. Also added a 'Flipped layout" per Jen!

Sending to @pameeela for review and assigning credit.

Going to take this for a whirl now...

I have some suggestions, but let me show the results of testing first.

Now some feedback... I understand this doesn't have to be perfect so feel free to ignore some/all of this... forgive the formatting... it's getting late here:

1. Naming conventions: Given we want to support backwards compatibility and avoid confusion, IMO we should make sure we are pretty happy with the naming conventions of the prop names
   1. Heading: called "Title / Headline" in Figma and "Heading" in prop. The hero MR is using "heading" and "subheading". So... leaving this has heading seems fine as long as we are consistent across components.
   2. Tagline: called "Subtitle / Tagline" in Figma. I don't necessarily agree with this naming, given this can be long. Per chatgpt: "A tagline is typically quite short, usually just a few words or a brief phrase. I'd rather see it as "summary" or "text" or "description", but if everyone is happy with this then [shrug]. The hero MR is using "subheading", but I don't think that makes sense as a subheading would normally be short (I personally wouldn't use subheading for the hero but [shrug])
   3. Button text and link: Figma uses "CTA" but "button" is more user-friendly, so seems fine. The hero MR uses the same. But these use "cta" and "ctahref" for their prop names... we could make these consistent like "button_text" and "button_link"
   4. Image: good :)
   5. Flip horizontal layout: okay, I guess, but maybe "Image position" (left/right) would be less jargon-y... no big deal
2. Slots: I agree that it would be hard for users right now to deal with slots so it seems okay to not use them for this
3. Prop types: these seem okay except the "tagline" should be a textarea IMO as it is likely to be longer than the heading
4. Attributes: this isn't needed and can be removed... these are automatically injected into the SDC

I'll try to make a couple of these changes that seem more important.

I implemented suggestions 3 & 4 from #46 as well as changed the status to "experimental" per other discussions (though maybe the status will be removed).

For more info on the attributes, see #3469525: Update SDDS SDCs attributes prop to align with XB.

If anything else from #46 should change, I can make the changes myself, just let me know.

Oh... and here's how the tagline "textarea" looks:

Forgot the mobile view...

If anything else from #46 should change, I can make the changes myself, just let me know.

Yeah! Feel free to get in here and make changes. Just ping me in slack so we're not duplicating work.

The one I'd like some feedback on is "tagline". Do people resonate with that name?

I was thinking "summary" would be simple and more accurate, but open to opinions.

If this changes, I'll change it here and on the hero to match.

UPDATE: I'm pinging some people in Slack and will work on some other issues in the meantime.

A tagline is a marketing device for an organisation. If you have company profiles it could be a field there but not for generic content or fields.

A title and a subtitle is a content field.

A heading of various levels are HTML elements that can be applied to content.

A button is an HTML element. A link should be labelled as a link. A class of button-styled-link may be applied, but still it’s a link.

The call to action is the whole component, part of the call to action is the description of what will happen when you follow the link.

So I would use naming along these lines:

Title
Call to action text
CTA Link
CTA Link text

Thanks @tonypaulbarker!

I'm worried that not everyone knows CTA... which is probably why it wasn't called that.

And, we are saying the links are optional, so there could be no CTA at all, so I wouldn't call the text anything to do with CTA.

Note that the description text could refer CTA, but right now the descriptions aren't on the form fields.

Given the above, I suggest we go for more generic naming, something like below, which can be used across components, so the terminology isn't changing for each one.

• Title
• Summary (or Text)
• Link Text
• Link URL

I think that would work. Summary feels accurate.

Cool, thanks. I'll update with that and then update the hero to match.

I pinged @mherchel in Slack to get sign-off on #53 before doing the naming changes as I don't want to put the effort in if he's not onboard.

@kristen pol those names sound good to me. I originally changed it from 'CTA' to 'Button' but I didn't spend much time on that, just had a feeling that CTA was not super obvious. Link is also good!

Sweet! Thanks! I'm unblocked now, and will move forward on this one and then updating all MRs to use a similar convention.

Here's the screenshots:

Props

Desktop

Mobile

Okay... I'm done with my tweaking :)

If this is looking okay, then I can use the same naming on other MRs.

It seems to be working as expected though I could not add an image on experience builder. But that might be just related to that.

This is looking great based on the screenshots! Don't wait for my review, happy for a front end tick to get this merged.

Alright, folks seem to agree this looks good and I can find nothing to complain about in the code. Sick of seeing this one languish in review forever, so I'm merging it.

Finally merged into 1.x, thanks!