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

Background Component Image Update #2385

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Conversation

colbytcook
Copy link
Contributor

Summary

Updated the Background Component to use the Image Element instead of the Image Component

Details

In order to allow for a more performant Hero, we need to remove all the Image Component. On of the steps to do that is to replace the Image Element from the Background Component so we can leverage more perforrmant methods (for example, appending loading="eager" to all images in a hero).

How to test

  • Review the code and pull down the branch.
  • Review the Bolt Background, and the Bolt Band docs.
  • Review the docs "Pages" folder

Release notes

The Background Component now leverages the Image Element instead of the Image Component. This is a step to create a more performant Hero

@github-actions github-actions bot added the type: feature List this PR in the 'Features' section of the release notes. label Nov 17, 2021
@colbytcook colbytcook requested a deployment to feature/DS-622-build-hero-with-layout--ef08a8dd--commit-preview November 17, 2021 19:56 In progress
@danielamorse danielamorse merged commit 2be9236 into master Nov 18, 2021
@danielamorse danielamorse deleted the feature/background-update branch November 18, 2021 18:46
danielamorse added a commit that referenced this pull request Nov 18, 2021
lazyload: item.lazyload ?? true,
cover: item.cover ?? true
}) only %}
{% if item.pattern == 'image' %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is nicely backwards-compatible, but what's the plan going forward? As it stands now, you have to pass data as expected by the deprecated image component. There's no way to do it "right" even if you know better. Annoying at best, but this may come back to bite us when we try to replace other image components with image elements in Drupal.

Here's a proposal-- what if within if item.pattern == image, we have a second check: if there's either a src or srcset prop, then we assume that the deprecated image component pattern is being used (since those props exist in image component but not image element). However, if neither of those props is present, we assume the user is actually passing data that's intended to be rendered with the image element.

Thoughts? We can handle this in a follow up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Following up on this, I realized we already have this logic at the bottom of this control statement:

{% elseif (item is iterable) %}
  <div class="{{ 'c-bolt-background__item' }}">
    {{ item }}
  </div>
{% else %}
  {{ item }}
{% endif %}

That allows you to just pass an already rendered (or renderable) item without resorting to the horrible pattern_template() design pattern. That is the preferred option for anyone who wants to use this pattern correctly. Our schema docs need some updates, but otherwise this is fine as-is.

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.

None yet

5 participants