Create new SDC component for Umami (Common Card)

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

Parent issue committed.

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

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.

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.

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!

@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
--content
----card
------card
------card-common
------card-alt
----teaser
------teaser
------teaser-large
----search-result
------search-result
------search-result-promoted
------search-result-event
------search-result-archived

Makes sense to me.

Provided some feedback on the MR

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

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.

I updated the parent issue to 🌱 [META] Create Single Directory Components for the Umami theme Active .

Working on this @ Drupalcon.

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.

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?

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.

Added some suggestions.

I made the suggested changes.

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?

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.

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

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

@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.

@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.

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?

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.

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.

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.

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

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.

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 please update MR to use 11.x as target branch instead of 10.1

Thanks @andypost !

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

Added `only`

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!

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

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

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