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

DS-767 Remove usage of image component from card #2520

Merged

Conversation

MarcinMr
Copy link
Collaborator

@MarcinMr MarcinMr commented Jul 29, 2022

Jira

https://pegadigitalit.atlassian.net/browse/DS-767

Summary

Support for image element was added.

Details

  • Support for the Image Element is added,
  • Backwards compatibility with the Image Component
  • Card schema was updated to point to the Image Element instead of the Image Component,
  • Image Component props were replaced with Image Element props in the pattern lab

How to test

Pull the branch. Check if by passing image element in the Card component to the media: { image: image

 {% set image %}
  {% include '@bolt-elements-image/image.twig' with {
    attributes: {
      alt: 'Image alt text',
      src: '/images/placeholders/landscape-16x9-mountains.jpg',
    },
  } only %}
{% endset %}

{% include '@bolt-components-card-replacement/card-replacement.twig' with {
  media: {
    image: image,
  },
  ...
  } only %}

renders the image element instead of the <bolt-image/> component

Release notes

When adding an image to the media inside Bolt Card component, please use a renderable image element for media.image instead of structured data.

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Jul 29, 2022
@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview July 29, 2022 12:15 Inactive
@colbytcook
Copy link
Contributor

@MarcinMr disregard the previous comment, for some reason when i pulled the branch it was out of date. Everything is looking correct now.

Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

This isn't quite what I was thinking. Apologies for not giving clearer direction on this in the ticket!

In general, we want to avoid coupling components together. I've added some docs to the wiki on the how and why: https://github.com/boltdesignsystem/bolt/wiki/Decoupling-Bolt-components

What that means in practice is that instead of duplicating what's already there (a hard-coded include of another component), I was thinking we could update the media template to look like this:

<bolt-card-replacement-media {{ attributes }}>
  <ssr-keep for="bolt-card-replacement-media" class="c-bolt-card_replacement__media">
    {{ image }}
  </ssr-keep>
</bolt-card-replacement-media>

What that actually would look like is something more like this, keeping support for all the deprecated/wrong ways of doing it.

<bolt-card-replacement-media {{ attributes }}>
  <ssr-keep for="bolt-card-replacement-media" class="c-bolt-card_replacement__media">
    {% if _card_replacement_image %}
      {# DEPRECATED.   Pass a renderable image for media.image instead of structured data.  #}
      {% if _card_replacement_image is iterable and  _card_replacement_image.src %}
        {% include '@bolt-components-image/image.twig' with _card_replacement_image only %}
      {% else %}
        {{ _card_replacement_image }}
      {% endif %}
    {% elseif block('media') is defined %}
      {# DEPRECATED.   Pass a renderable image for media.image instead of using a twig block.  #}
      {{ block('media') }}
    {% endif %}
  </ssr-keep>
</bolt-card-replacement-media>

@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview August 16, 2022 10:24 Inactive
…o feature/DS-767-Remove-usage-of-image-component-from-card
@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview August 16, 2022 10:52 Inactive
@MarcinMr
Copy link
Collaborator Author

@remydenton

Thanks for explaining, this was my misunderstood. I updated the template and PL files.

I have one more question regarding _card-replacement-media.twig

On line 16 we check if the Card component has horizontal prop and then we update the props with the ratio and cover (this is for the deprecated).

On line 24 I tried to do the same for the image-element, Whenever the horizontal prop is true I wanted to update the image element with the prop background: true (needed for cropping Image Element when horizontal card is used)

But the terminal throws an error:
error - updating object

Seems like I can't use the merge filter on that object?

So whenever we want horizontal cards with image elements we need to add background: true to the image element include:

{% include '@bolt-elements-image/image.twig' with {
  background: true,
  attributes: {
    ...
  },
} only %}

Is there a way to do it in the template? Or adding the background prop directly to the image-element include is ok?

@MarcinMr MarcinMr requested a review from remydenton August 16, 2022 10:56
@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview August 16, 2022 11:18 Inactive
Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Looking good @MarcinMr, I left a couple minor inline questions (though maybe you were still working on these).

Re: updating the ratio and cover props when horizontal is true, there is no way to do it automatically, and that's by design (or at least part of the decoupling principle). Instead, you just want to set those additional properties manually in the image that gets passed any time horizontal is true. We should also update the docs to tell users that those image props are necessary for horizontal cards.

@@ -5,7 +5,7 @@
image: {
src: '/images/placeholders/landscape-16x9-mountains.jpg',
alt: 'Image description',
lazyload: false,
loading: 'lazy',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason you didn't replace the image structured data array here with a rendered image, such that this looks like the following?

media: {
  image: image,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@@ -644,7 +644,6 @@
media: {
image: {
src: '/images/content/promos/promo-16x9-anthem.jpg',
placeholder_color: '#8d9598'
Copy link
Collaborator

Choose a reason for hiding this comment

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

These shouldn't be structured arrays either, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@MarcinMr
Copy link
Collaborator Author

@remydenton thanks for explaining. I updated the docs and the pattern lab, so right now there shouldn't be more examples like this:

media: {
  image: {
    src: '/path/to/image,

@colbytcook colbytcook requested a deployment to feature/DS-839-editable-profile-avatar--625f045--commit-preview August 22, 2022 13:06 In progress
@MarcinMr MarcinMr requested a review from remydenton August 22, 2022 13:12
@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview August 22, 2022 13:18 Inactive
@@ -32,6 +53,14 @@
<bolt-text headline>
Default horizontal card
</bolt-text>
<bolt-text>
When using background element set <code>background: true</code>. This will crop the image in the horizontal card.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Helper text here is just what we need, but I'm not sure I understand from the sentence how it's supposed to work. There isn't a background element here 🤔 . Isn't it essentially just the following?

When creating a horizontal card by setting the horizontal prop to true, the image you pass for card media should also have its background prop set to true in order to allow the image to be cropped and display properly.

Feel free to tweak the wording, but I think the really important thing to get across is that users need to set background: true on the image element whenever the card has horizontal: true.

Copy link
Collaborator Author

@MarcinMr MarcinMr Aug 23, 2022

Choose a reason for hiding this comment

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

Today when I read this sentence, I didn't understand it either 😁 I updated the docs with your more specific description, only added one more detail that the horizontal card also must have the prop height set to auto. Without this images are broken.

@remydenton
Copy link
Collaborator

Looking really good, just one final comment/question on the wording in the docs.

@MarcinMr MarcinMr requested a review from remydenton August 23, 2022 09:06
@colbytcook colbytcook temporarily deployed to feature/DS-767-Remove-usage-of-image-component-from-card--56631e0--commit-preview August 23, 2022 09:10 Inactive
@colbytcook colbytcook requested a deployment to feature/DS-767-Remove-usage-of-image-component-from-card--56631e0--commit-preview August 23, 2022 09:16 In progress
@colbytcook colbytcook requested a deployment to feature/DS-767-Remove-usage-of-image-component-from-card--9e9619d--commit-preview August 23, 2022 09:30 Abandoned
@colbytcook colbytcook requested a deployment to feature/DS-767-Remove-usage-of-image-component-from-card--branch-preview August 23, 2022 09:37 Abandoned
Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the work on this @MarcinMr .

@remydenton remydenton merged commit b86f5c2 into master Aug 23, 2022
@remydenton remydenton deleted the feature/DS-767-Remove-usage-of-image-component-from-card branch August 23, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature List this PR in the 'Features' section of the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants