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.

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

Testing instructions

Make sure you have SDC module enabled, if enabling via drush si demo_umami it will be installed by the profile
Clear caches
Test umami footer blocks - you should see HTML like the screenshot below showing that the block is being rendered via a component.

Components description

Common Footer Block contains:

1. Optional image
2. Title
3. Content

Title component contains:
1. specific Drupal properties: title_prefix, title_suffix
2. optional html tag as property
3. In the future, it will allow us to control the typography of the site through css classes. EG: `.umami-title--h1 {}` what is better that `h1 {}`
In terms of BEM: https://en.bem.info/methodology/quick-start/#block `You also shouldn't use CSS tag or ID selectors when using BEM.`

Optional follow up

Create follow up issue for footer menu, Which seems equal in terms of CSS with recipe tags list over this footer.

Issue fork drupal-3368102

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:

Comments

Gauravvvv created an issue. See original summary.

gauravvvv’s picture

Status: Active » Needs review
StatusFileSize
new5.13 KB

I have attached the patch with new component for menu footer. please review

smustgrave’s picture

Category: Support request » Feature request
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Verified on 11.x that the block renders the same as before.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    @@ -0,0 +1,45 @@
    +# Everything in this file is optional. Still, the file needs to exist. Adding
    

    Should we remove comments here? Seems like a lot to include all of this in every component.

  2. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.twig
    @@ -0,0 +1,25 @@
    +    'block-' ~ configuration.provider|clean_class,
    

    Shouldn't configuration be documented in the schema? Wondering if this should be triggering schema validation 🤔

  3. +++ b/core/profiles/demo_umami/themes/umami/templates/components/navigation/block--system-menu-block--footer.html.twig
    @@ -1,25 +1,9 @@
    +    title_attributes: title_attributes,
    

    Should we use only here to avoid passing down context?

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new5.23 KB
new1.86 KB

Addressed feedback in #4, attached interdiff for same. please review

smustgrave’s picture

Status: Needs review » Needs work

+{{ dump(configuration)}}

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new661 bytes
new5.21 KB

Removed dump, attached interdiff. please review

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Seems points in #4 have been addressed.

e0ipso’s picture

Status: Reviewed & tested by the community » Needs work

My main concern is about coupling between the component and the host template. This is an issue that happens when refactoring existing code, and will likely be less so when writing components from scratch.

  1. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    @@ -0,0 +1,48 @@
    +$schema: https://git.drupalcode.org/project/sdc/-/raw/1.x/src/metadata.schema.json
    

    $schema: https://git.drupalcode.org/project/drupal/-/raw/10.1.x/core/modules/sdc/...

  2. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    @@ -0,0 +1,48 @@
    +name: Umami footer
    

    It took me a moment to find this, so I am posting it.

    https://www.drupal.org/docs/develop/user-interface-standards/interface-t...

    It seems this is the correct capitalization (sentence like), according to our style guide.

  3. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    @@ -0,0 +1,48 @@
    +    configuration:
    +      type: array
    +      label: configuration
    +    plugin_id:
    +      type: string
    +      label: Plugin Id
    

    Accordig to the use of this below, this is not an array but an object.

    Also, I believe we should simplify this and skip this. This couples the component to a block, which is not desirable. Moreover, this is only used to build up a set of classes.

    IMHO we should have a prop that allows the host template pass additional classes if we need them. Hence the block will send the provider, but something else will not.

    Same principle applies to the plugin_id.

  4. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    @@ -0,0 +1,48 @@
    +    type: string
    

    This is an invalid key. Slots do not provide an schema, only some basic metadata.

    Maybe replace with:

      content:
        title: Content
    
  5. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.component.yml
    similarity index 100%
    rename from core/profiles/demo_umami/themes/umami/css/components/navigation/menu-footer/menu-footer.css
    
    rename from core/profiles/demo_umami/themes/umami/css/components/navigation/menu-footer/menu-footer.css
    rename to core/profiles/demo_umami/components/menu-footer/menu-footer.css
    

    The menu-footer.css should be free of references to host template classes.

  6. +++ b/core/profiles/demo_umami/components/menu-footer/menu-footer.twig
    @@ -0,0 +1,25 @@
    +    'block',
    +    'block-' ~ configuration.provider|clean_class,
    +    'block-' ~ plugin_id|clean_class,
    +    'menu-footer-wrapper',
    

    The internal component template should be independent from where it's placed. IMO we should not reference Drupal constructs like "block", or "menu".

    This same comment may apply elsewhere in the patch.

  7. +++ b/core/profiles/demo_umami/themes/umami/templates/components/navigation/block--system-menu-block--footer.html.twig
    @@ -1,25 +1,12 @@
    +{% embed 'demo_umami:menu-footer' with {
    +    attributes: attributes,
    +    title_prefix: title_prefix,
    +    title_suffix: title_suffix,
    +    label: label,
    +    title_attributes: title_attributes,
    +    content: content,
    +    configuration: configuration,
    +    plugin_id: plugin_id,
    +} only %}
    +
    +{% endembed %}
    

    Here we are not passing the content slot as a twig block, instead we are passing it as an undocumented prop.

    Also, we should use the simplified syntax when the label and the name of the variable are the same.

      foo: foo,
    

    becomes

      foo,
    
gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new4.72 KB
new4.45 KB

I have addressed above feedback, only point 6 is left. Attached interdiff for same

smustgrave’s picture

Status: Needs review » Needs work

For point 6 in #9 didn't test yet.

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

finnsky’s picture

StatusFileSize
new335.45 KB

What i see now. We have
- 2 footer blocks which contains title and content
- 2 equal menus (footer menu and recipe tags)

components in footer

Will try with this approach

finnsky’s picture

Pushed footer block. Gonna check menu in separated commit.

finnsky’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work

CC failure.

Also if #3367152: Create new SDC component for Footer promo is being combined will need an issue summary update.

finnsky’s picture

Status: Needs work » Needs review

Added `blockfooter` to CSpell dictionary.

andypost’s picture

There's already blockrecipe in dictionary so it's ok to update it with new block

andypost’s picture

smustgrave’s picture

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

Closed #3367152: Create new SDC component for Footer promo as a duplicate.

Tagging for issue summary update now.

finnsky’s picture

Title: Create new SDC component for Umami (umami footer block) » Create new SDC component for Umami (umami common footer block)
Issue summary: View changes
finnsky’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update
finnsky’s picture

Issue summary: View changes
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new461.36 KB
new389.47 KB

Before

before

After

after

So only difference I see is that the image is smaller but actually seems better. Think this is good.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Posted comment on the MR

smustgrave’s picture

@lauriii this good to be marked back to RTBC?

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Going to remark but if you get a chance @lauriii

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

Moving to needs review to resolve the discussion in the MR

finnsky’s picture

smustgrave’s picture

Believe random familiar. Rerunning tests.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new507.67 KB
new433.36 KB

As I thought was random

Before

before

After

after

May need submaintainer sign off but the image changed in sign, see before/after screenshots. But personally I think it looks better and actually shows the full image, so marking it.

finnsky’s picture

Rebased.

e0ipso’s picture

+1 that the image looks better now.

  • lauriii committed 0852886c on 11.x
    Issue #3368102 by finnsky, Gauravvvv, smustgrave, lauriii, e0ipso,...

  • lauriii committed 407fb39c on 10.2.x
    Issue #3368102 by finnsky, Gauravvvv, smustgrave, lauriii, e0ipso,...

lauriii’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 0852886 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

Status: Fixed » Closed (fixed)

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