This is a regression from #3173008: [Code Review] wide image within article template a reusable component/class

In that patch we add the wide-image CSS class based on the field type, view mode, and is_multiple.

Evidently, we also need to check for the region.

Problem/Motivation

Original post

I updated my website to Drupal 9.2.1 and this update brings a lot of bugs to the Olivero theme.

Responsive images did not work before, but this is even worse.

The "Hero" region spills over into the "Social Bar" region. The icons in the "Social Bar" region are no longer fixed, they will disappear if I scroll the page.

Steps to reproduce

  1. Install the theme and enable it.
  2. Create a custom block type for eg (hero banner). Add a field image in it and save it.
  3. Add a custom block (upload an image) and save it.
  4. Place the block in to Hero (full width) region and save it.
  5. You will see the issue.

Proposed resolution

Image has extra margin-inline-start inside
.region--hero .wide-content
Reset the margin-inline-start for the .wide-content inside .region--hero only with the following code

region--hero .wide-content {
  max-width: 100%;
  margin-inline-start: 0;
  margin-inline-end: 0;
}

Remaining tasks

User interface changes

Before

before-patch

After

after-patch

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#18 before-patch.png5.82 MBshweta__sharma
#18 after-patch.png6.1 MBshweta__sharma
#2 Le_web_made_in_France___cocoriNET.png988.08 KBmherchel
Capture d’écran de 2021-07-08 12-32-07.png349.4 KBzenimagine
Capture d’écran de 2021-07-08 12-32-01.png404.96 KBzenimagine
Capture d’écran de 2021-07-08 12-31-26.png3.15 MBzenimagine
Capture d’écran de 2021-07-08 12-31-08.png2.69 MBzenimagine

Issue fork drupal-3222784

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

zenimagine created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Region "Hero" is not displayed correctly in version 9.2.1 » Olivero: 'wide-image' utility class breaks layout when placed in 'Hero" region
Version: 9.2.x-dev » 9.3.x-dev
Issue summary: View changes
Issue tags: +CSS
StatusFileSize
new Le_web_made_in_France___cocoriNET.png988.08 KB

Thanks for finding this bug!

On our dev demo site, the hero region is displaying properly (see https://tugboat-aqrmztryfqsezpvnghut1cszck2wwasr.tugboat.qa/node/23)

I was able to grab the the URL of your live site within your screenshot and inspect the CSS. It looks like this is a regression from #3173008: [Code Review] wide image within article template a reusable component/class

In that patch we add the wide-image CSS class based on the field type, view mode, and is_multiple.

Evidently, we also need to check for the region.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: Olivero: 'wide-image' utility class breaks layout when placed in 'Hero" region » Olivero: 'wide-image' utility class breaks layout when placed in 'Hero' region
zenimagine’s picture

zenimagine commented

@mherchel I just tested again and the image does not overflow if I am an anonymous user, but it does overflow if I am logged in as administrator.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Priority: Normal » Minor
xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Minor » Normal

Normal priority bug under the normal issue priority definition:

Bugs for site visitors that do not interfere with site use, for example, visual layout issues.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

Can we handle it on css level, since it's easier?

Because field entity doesn't know anything about in which region its parent entity (block_content in our case) rendered. For sure there is a way to try to catch region name from field preprocess still, but we can still have feedbacks later, because it can be not block_content, but some custom block, or it can be view instead -> which means i'm not sure if we can control all possible cases to understand if our image is in hero region or not.

Maybe we can provide additional setting to the fields globally with region name where this field is rendered? Is it possible actually?

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

Can be related https://www.drupal.org/project/drupal/issues/3211651 if

Maybe we can provide additional setting to the fields globally with region name where this field is rendered?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

Commenting that I experienced something similar with primary-image in field--node--field-image.html.twig

I placed nodes in a grid 3 wide and the images got a bit messy.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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.

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

Gauravvvv opened merge request !5572

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review

Image has extra margin-inline-start inside .region--hero .wide-content. I have removed the margin-inline-start for the .wide-content inside .region--hero only. I have created MR for same. please review

smustgrave’s picture

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

Issue summary is missing standard template with proposed solution, steps to reproduce, before/after screenshots.

shweta__sharma’s picture

shweta__sharma commented
Issue summary: View changes
StatusFileSize
new after-patch.png6.1 MB
new before-patch.png5.82 MB

Issue summary updated.

shweta__sharma’s picture

shweta__sharma commented
Title: Olivero: 'wide-image' utility class breaks layout when placed in 'Hero' region » Olivero: 'wide-content' utility class breaks layout when placed in 'Hero' region

MR 5572 has fixed the issue. We can now move this to RTBC.
Thanks

shweta__sharma’s picture

shweta__sharma commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update
quietone’s picture

quietone commented

I'm triaging RTBC issues.

@shweta__sharma, thank you for completing the issue summary, that really helps with reviews.

I read the IS and the comments. I don't see any response to the suggestions by @kostyashupenko in #7 and #8.

Leaving at RTBC.

nod_’s picture

nod_
Primary language French
Location Lille
commented
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

There's already similar image-overlap-protection for other use cases in wide-content.pcss.css, see that here. The similar code being added here should go in there, perhaps as an additional selector to a style that is already there.

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

ahsannazir’s picture

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

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

1 small comment on MR. But if different solution is being used can issue summary be updated to reflect.

ahsannazir’s picture

ahsannazir commented
Issue summary: View changes
ahsannazir’s picture

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

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Believe #23 has been addressed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

comment #23 was not taken into account

ahsannazir’s picture

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

smustgrave commented
Status: Needs review » Needs work

For the feedback from @mherchel.

ahsannazir’s picture

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

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

Changes to core/themes/olivero/css/layout/region-hero.css may be out of scope. But it's a small change so may not be a big deal.

  • nod_ committed 8756d339 on 11.x 
    Issue #3222784 by ahsannazir, Gauravvvv, zenimagine, shweta__sharma,...

nod_ closed merge request !5572

  • nod_ committed 747d12b9 on 10.3.x 
    Issue #3222784 by ahsannazir, Gauravvvv, zenimagine, shweta__sharma,...

  • nod_ committed 00edfd5f on 10.2.x 
    Issue #3222784 by ahsannazir, Gauravvvv, zenimagine, shweta__sharma,...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 8756d33 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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