Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(card): component renders multiple headings #12030

Conversation

Valentin-Sorin-Nicolae
Copy link
Contributor

@Valentin-Sorin-Nicolae Valentin-Sorin-Nicolae commented Sep 16, 2024

Related Ticket(s)

ADCMS-6175
Closes #12000

Description

C4DCard and C4DCardCTA both have a _renderHeading() method that returns the following:

<slot name="heading"></slot>
<c4d-card-heading>${caption}</c4d-card-heading>

If these components are authored with a element within them, it's automatically assigned to slot="heading". This leads to cards with multiple headings, each with their own minimum of 64px of margin-bottom.

image

Changed

image

@Valentin-Sorin-Nicolae Valentin-Sorin-Nicolae requested a review from a team as a code owner September 16, 2024 10:21
@Valentin-Sorin-Nicolae Valentin-Sorin-Nicolae added bug Something isn't working and removed bug Something isn't working labels Sep 16, 2024
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2024

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2024

@andy-blum andy-blum requested review from a team, sangeethababu9223, kennylam and annawen1 and removed request for a team and sangeethababu9223 September 18, 2024 12:35
@andy-blum andy-blum self-requested a review September 18, 2024 13:12
@RichKummer
Copy link

Hey @Valentin-Sorin-Nicolae , two notes on this:

  1. The original Storybook styles were off on this one – the video caption (if there is one) should really be coming in as body copy in the body copy section of the card, instead of a second header. The intent is that the caption would render as the body copy.
  2. What you could do is set it up so that if a video caption exists and the person adds body copy, the body copy would override the incoming video caption (so only the body copy would display). If no body copy is added, then the video caption displays. If neither the body copy or caption info exists, then no body copy is displayed.

@m4olivei m4olivei added the bug Something isn't working label Sep 23, 2024
@m4olivei m4olivei added dev Needs some dev work adopter: AEM used when component or pattern will be used by this adopter owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants labels Sep 23, 2024
@m4olivei
Copy link
Contributor

Hey @RichKummer thanks for the review.

The original Storybook styles were off on this one – the video caption (if there is one) should really be coming in as body copy in the body copy section of the card, instead of a second header. The intent is that the caption would render as the body copy.

I'm not sure this is the case. What we're calling "caption" here, is really the videoName from the Kaltura API. It seems like the intent is for the videoName to be used as a fallback when no Custom title is provided. In which case, what we have here should suffice to fix the issue of a double title when a videoName is available.

Does that make sense? As I read over the issue and code, it seems to me that the naming of caption is causing some confusion. Maybe we should make an adjustment there as well.

@RichKummer
Copy link

@m4olivei Ah okay, if that is the case then I think it's fine to have videoName come in as the heading if it is available, or overridden by a custom heading if one exists. In either case, there should only ever be one heading.

The name caption is definitely confusing if it's pulling the title from Kaltura, I agree that the prop name should be updated to be clear.

Copy link

@RichKummer RichKummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@m4olivei m4olivei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me as well. Will add the Ready to Merge label.

@m4olivei m4olivei added the Ready to merge Label for the pull requests that are ready to merge label Sep 30, 2024
@kodiakhq kodiakhq bot merged commit e715924 into carbon-design-system:main Sep 30, 2024
11 of 18 checks passed
Copy link
Contributor

github-actions bot commented Nov 6, 2024

Hey there! This issue/pull request was referenced in recently released v2.14.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adopter: AEM used when component or pattern will be used by this adopter bug Something isn't working dev Needs some dev work owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[card, card-cta]: Component renders multiple headings
5 participants