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.

This issue is of course blocked on actually getting SDC into core. See #3340712: Add Single Directory Components as a new experimental module.

Testing instructions

  1. Make sure you have SDC module enabled, if enabling via drush si demo_umami it will be installed by the profile
  2. Clear caches
  3. Test card view mode and it's variants (most of the listings use it) - you should see HTML like the screenshot below showing that the cards are being rendered via a component

Umami card, using SDC for rendering

Components description:

Card Component:

1. Has optional html_tag prop with values article or div
2. Has slot content without restrictions of content inside.

Title component:
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.`

Read more component used only in cards for now. But it can be reused everywhere now.

CommentFileSizeAuthor
#41 3347672-nr-bot.txt90 bytesneeds-review-queue-bot
#39 3347672-nr-bot.txt90 bytesneeds-review-queue-bot
#10 Screenshot 2023-04-22 at 7.41.25 PM.png4.05 MBsmustgrave
#10 Screenshot 2023-04-22 at 7.41.19 PM.png2.66 MBsmustgrave
#8 umami-card-sdc.jpg466.17 KBmarkconroy

Issue fork drupal-3347672

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

markconroy created an issue. See original summary.

markconroy opened merge request !3657

markconroy’s picture

markconroy commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed
Issue tags: +Needs Review Queue Initiative

Only postponing until the parent issue makes it in. Since this can be looked at but it can't be truly reviewed (as the parent may need changes) or move forward.

Tagging for Needs Review Queue though so when the parent gets committed and this gets moved back to NW we can prioritize

ipwa’s picture

ipwa commented
Status: Postponed » Needs review

Parent issue committed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Imagine the MR will have to rebased. Current MR has some failures.

markconroy’s picture

markconroy commented
Status: Needs work » Needs review
markconroy’s picture

markconroy commented
Issue summary: View changes
StatusFileSize
new umami-card-sdc.jpg466.17 KB
markconroy’s picture

markconroy commented

It was mentioned on Slack that Umami maintainers do not want to allow the use of experimental modules in the umami profile.

I had forgotten that we weren’t going to use experimental features, I got so excited about using SDC. The fear is that if we use experimental modules

  1. They are not encouraged for general users, and
  2. They might be removed from Drupal core if they don’t become stable.

However, given that experimental modules in reality don't seem to actually get removed and given there are not concerns related to data storage in the SDC module (it's only templating changes), I'm happy as an Umami maintainer for us to override our policy on experimental modules for SDC.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot 2023-04-22 at 7.41.19 PM.png2.66 MB
new Screenshot 2023-04-22 at 7.41.25 PM.png4.05 MB

Won't hold up but applying patch I get

test.patch:73: trailing whitespace.

test.patch:88: trailing whitespace.

warning: 2 lines add whitespace errors

Applied the patch on a fresh Umami install and checking the cards on the homepage nothing seemed to change. Attaching some screenshots.

1

2

Reviewing the MR the changes look good and right along the time of component/atomic structure.

Let me know if there's anything else you want me to check!

markconroy’s picture

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

@smustgrave Thanks for that review and moving to RTBC. I've actually gone and updated the MR to move all the card components to their own directory, so I can (later) add in more directories beside it for other view modes. Can you give it another review?

New directory structure will allow us to do something like this:

components
--regions
----header
----footer
----sidebar
--content
----card
------card
------card-common
------card-alt
----teaser
------teaser
------teaser-large
----search-result
------search-result
------search-result-promoted
------search-result-event
------search-result-archived
etc, etc, etc
smustgrave’s picture

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

Makes sense to me.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work

Provided some feedback on the MR

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

moshe weitzman’s picture

moshe weitzman commented

Looks like we lost some steam here, right near the end. Anyone available to address the feedback?

markconroy’s picture

markconroy commented

Sorry @moshe weitzman

I've just been snowed under with work for the past couple of weeks and haven't had a chance to get back to this yet.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Issue tags: +SDC Sprint DrupalConNA 2023
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Parent issue: #3313520: Single-directory components in core » #3364673: [META] Create Single Directory Components for the Umami theme
Related issues: +#3313520: Single-directory components in core

I updated the parent issue to #3364673: [META] Create Single Directory Components for the Umami theme.

mortona2k’s picture

mortona2k commented
Assigned: Unassigned » mortona2k

Working on this @ Drupalcon.

mortona2k’s picture

mortona2k commented

1. Classes
I moved the classes array out of the component templates and into node templates. I think they can also be safely removed from the node templates. If we keep them in, it will inject a bunch of classes potentially for future use, but currently aren't used.

One exception is .view-mode-card. I don't think this is an appropriate class to use in a component, because it targets a view mode named card, not a card component, which we are creating. Instead, I used .umami-card, with .umami-card--common and .umami-card--common-alt variations.

I changed node__title and node__content to umami-card__title and umami-card__content.

2.1 Title
I used a slot for the title, but have an h2.umami-card__title element in the component, which wraps it.
This way, the component template defines the h2 tag, and the content can be whatever is passed through. In this case, it's a span, with a bunch of classes on it.
Having done this, I am leaning towards stripping things down to just what is needed for the component instead of trying to support unknown variations.

mortona2k’s picture

mortona2k commented
Status: Needs work » Needs review

I could use some feeback on this.

Last round of changes make the card--common and card--common-alt components extend the card template.

Any thoughts on how we're using props and slots?

markconroy’s picture

markconroy commented

Thanks @mortona2k

This looks really good now. I've only read through the code, not actually tested it yet. I'll try get it tested later this week.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Needs work

Added some suggestions.

mortona2k’s picture

mortona2k commented
Status: Needs work » Needs review

I made the suggested changes.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I didn't check the changes (and I don't think I am qualified to talk about CSS), but I still see several unresolved threads in the MR. Did you consider those as well?

mortona2k’s picture

mortona2k commented

I made the changes I could for those. I marked some of them as pending review, not sure if I'm doing that right.

I have a few questions on some of the threads, otherwise it's as close as I can get atm.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems the failures are from passing NULL in {{ node.type.entity.label() }}

Wonder if you have to pass that in as a slot value?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

@smustgrave if the component is embedded without context leaks, then you need to pass the label as well. IMHO a card component should not depend on a node object, but a label seems reasonable. In any case, mapping it into context should do.

smustgrave’s picture

smustgrave commented

@mortona2k do you want to give that a shot? So I can stay in the review role.

Seems the only failures in the MR are due to that node object being empty.

mortona2k’s picture

mortona2k commented
Status: Needs work » Needs review

I added a read_more_label property, set with node.type.entity.label() in the node templates.

Do I need to set a default value, or does defining the property take care of that?

smustgrave’s picture

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

I don't think a default is needed but will let committer decide.

Think this is actually good to go!
Applied the MR and compared the cards on the homepage vs without the MR and they appear unchanged.

e0ipso’s picture

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

I left a bunch more comments. I think we are getting very close. Thanks for figuring all this out together!

Some of the comments on the metadata apply to all the components in here.

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

gauravvvv’s picture

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

smustgrave commented
Status: Needs review » Needs work

Seems a number of open threads. If some are done can they least be marked with a comment. Know you can't always resolve the thread.

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

finnsky opened merge request !4652

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

Hello all!

I'm happy that SDC in core because i'm big fan of components development. So:

I created new MR with different approach:

- From my point of view components should be abstract and not related to Drupal view modes, fields, content types or whatever.
- All that 3 cards absolutely same. So what for to create 3 components?
- Read More link should be independent component. It is reusable now. even if used only in cards.
- Title also pretty useful component in terms of typography. I added it without CSS because typography not designed well yet.

Minor updates:
- Changed spacing policy to CSS Grid. Some small visual layout regressions appear. But i don't think they are critical.
- BEMified components to have cleaner styles inheritance.
- removed label-items class in theme preprocess. It has default margin-bottom. What is bad in terms of BEM:
https://en.bem.info/methodology/css/#external-geometry-and-positioning

Please review!
Thanks

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3347672-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3347672-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@finnsky please update MR to use 11.x as target branch instead of 10.1

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

Thanks @andypost !

gauravvvv’s picture

gauravvvv commented
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: mortona2k » Unassigned
Status: Needs review » Needs work

Moving back to Needs Work, since there are a bunch of unresolved tasks.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

Added `only`

smustgrave’s picture

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

For review purposes can the issue summary be updated to match what's in the MR. Current IS talks about a card component which resulted in a title, readmore, and card components. Think that should be documented.

Thanks this looks really good!

finnsky’s picture

finnsky commented
Title: Create new SDC component for Umami (card view mode) » Create new SDC component for Umami (Common Card)
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
finnsky’s picture

finnsky commented
Issue summary: View changes
smustgrave’s picture

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

Applied the MR and verified that the cards still render the same as before.

  • justafish committed aac1528b on 11.x 
    Issue #3347672 by mortona2k, finnsky, markconroy, Gauravvvv, smustgrave...

  • 22c836de committed on 11.x 
    Create new SDC component for Umami (card view mode) - #3347672

  • justafish committed 2508928a on 10.2.x 
    Issue #3347672 by mortona2k, finnsky, markconroy, Gauravvvv, smustgrave...
justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 11.x and 10.2.x - thanks!

Status: Fixed » Closed (fixed)

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