Create new SDC component for Umami Banner

I have created component for Umami banner. please review

Seemed to cause some failures. Most likely due to SDC not being installed in Umami yet but would need to confirm first before reviewing.

Tested on a fresh Umami install.

Before

After

I don't see any visual regression caused by the move.

Actually is the components folder suppose to go outside the theme?

This is going to conflict with 📌 Improve Largest Contentful Paint in Umami front page Fixed , although it'd be kind of interesting to see what the component diff looks like assuming this lands first.

I added some suggestions. As in other similar reviews, I did not look at CSS and markup, since I am not qualified for it.

Hello all!

- Branch was pretty outdated so it was easier to create new one.
- I used slots because now it is responsive image in umami.
- One css change on board: `.banner-block` => `.banner`. Because it is component now and not a block.

Please review!

Very nice work. Thanks very much.

Same deal as the other one. Can the issue summary be updated for the new components be added. IS mentions just the banner.

@e0ipso if you get a change believe there are some open questions in the thread from @finnsky.

Thanks for taking care of that, removing tag.

Applying MR 4651 I can confirm the banner on the homepage still renders the same

We should probably avoid Field related markup & classes inside components. This data might come from anywhere.

Great catch by @ e0ipso, didn't even think about not using that kind of css in components.

We might want to consider checking for non-empty content. I've provided an example where the block contains no content at all, and it appears unusual. I'm uncertain if this issue could potentially prevent us from merging this component, so I haven't changed the task status and have left this note for your attention.

Rebased. Please review!

RE: #20

Thank you for review!
But if you check with 11.x without content you will get same result without content :)

Can be follow up issue maybe. Set some fields as required. But obviously out of scope current ticket

finnsky → changed the visibility of the branch 11.x to hidden.

RTBC!

Let's get this merged!

Some little feedbacks following @finnsky request:

Source: https://git.drupalcode.org/project/drupal/-/blob/b8f0da6634683c886fbd6d7...

Error: Use attribute object

Default attributes object is always injected in template and expected for alteration (adding contextual accessibility attributes, adding data for site building tools, style overrides of the component...)

However it is not used in template:
<div class="banner">

Advice: Required slots?

slots:
  content:
    title: Content
    required: true
    description: This is the banner content
  image:
    title: Image
    required: true
    description: This is the banner image

I believe it is better to avoid required slots (which is a nice feature for very specific cases) as much as possible:

• In a "philosophical" point of view, slots are supposed to be as free as possible (free content, free usage)
• AFAIK, the requirement is not enforced at Render API level and we can't be sure site building tools will enforce it

Maybe this is related to the Mateu's feedback:

Is it OK to retain the empty divs if the slot are empty?

RE: #27

Fixed feedbacks. Thank you!

nod_ → closed merge request !4651

There is a 8px difference in width and a 4px difference in height (both bigger on the new component) on the call to action button. I don't think it's an issue so committing as-is.

Committed ff39253 and pushed to 11.x. Thanks!

It doesn't cherry pick to 10.2.x so not adding it there.

Automatically closed - issue fixed for 2 weeks with no activity.