Problem/Motivation

Single directory components (SDC) is a new way to theme Drupal. Instead of scattering related files around your theme, they're contained to one directory. The primary issue for SDC is at #3313520: Single directory components in core.

The Demo Umami team has decided that their theme can depend on an experimental module like SDC. This allows us to have the work in this ticket merged even when #3352256: [META] Move code from the experimental SDC module to core is not committed.

Banner component

Banner component contains:

1. Slot for image(any media)
2. Slot for content.

Button styles moved to `button--primary` variant.

As part of SDC's roadmap (see #3345922: Single Directory Components module roadmap: the path to beta and stable), we want to convert Umami components to use SDC. For this task I'm choosing the Banner component, which includes markup and CSS.

This component is a good fit because it already used in 2 places.

This component involves the following Twig templates, CSS, JS, assets, and libraries:

  1. core/profiles/demo_umami/themes/umami/css/components/blocks/banner/banner.css

Testing instructions

  1. Visit homepage

CommentFileSizeAuthor
#20 umami_test.png79.6 KBsnig
#7 after-3365497.png6.02 MBsmustgrave
#7 before-3365497.png6.6 MBsmustgrave

Issue fork drupal-3365497

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sarahjean created an issue. See original summary.

Gauravvvv made their first commit to this issue’s fork.

Gauravvvv opened merge request !4177

gauravvvv’s picture

gauravvvv commented
Component: theme system » Umami demo
Status: Active » Needs review

I have created component for Umami banner. please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative
StatusFileSize
new before-3365497.png6.6 MB
new after-3365497.png6.02 MB

Tested on a fresh Umami install.

Before

before

After

after

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

smustgrave’s picture

smustgrave commented

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

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

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

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Needs work

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

finnsky made their first commit to this issue’s fork.

finnsky opened merge request !4651

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

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!

markconroy’s picture

markconroy commented
Status: Needs review » Reviewed & tested by the community

Very nice work. Thanks very much.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Needs work
finnsky’s picture

finnsky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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.

finnsky’s picture

finnsky commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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.

snig’s picture

snig commented
StatusFileSize
new umami_test.png79.6 KB

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.

example test

finnsky’s picture

finnsky commented

Rebased. Please review!

finnsky’s picture

finnsky commented

RE: #20

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

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

finnsky’s picture

finnsky commented
Issue summary: View changes
finnsky’s picture

finnsky commented
Issue summary: View changes

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

markconroy’s picture

markconroy commented

RTBC!

Let's get this merged!

pdureau’s picture

pdureau commented

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?

finnsky’s picture

finnsky commented

RE: #27

Fixed feedbacks. Thank you!

  • nod_ committed ff392533 on 11.x 
    Issue #3365497 by finnsky, Gauravvvv, sarahjean, smustgrave, snig,...

  • nod_ committed 9d0913c2 on 10.3.x 
    Issue #3365497 by finnsky, Gauravvvv, sarahjean, smustgrave, snig,...

nod_ closed merge request !4651

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

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.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev

Status: Fixed » Closed (fixed)

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