The hero image for the article content type requires a large 1090px wide image style. We can define this image style within the theme (see https://www.drupal.org/docs/8/theming-drupal-8/including-default-image-s...).

We need to
1) Define this image style within the theme
2) Configure the field to use this image style

Styling will be handled in a separate issue.

CommentFileSizeAuthor
#10 3123348-10-image-style.patch1.5 KBmherchel
#9 3123348-8-image-style.patch1.5 KBmherchel
#6 3123348-6.patch1.51 KBkostyashupenko
#3 3123348-3.patch539 byteskostyashupenko
Olivero_Theme_-_Public_–_Figma-2.png638.51 KBmherchel

Comments

mherchel created an issue. See original summary.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: Unassigned » kostyashupenko
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: kostyashupenko » Unassigned
Status: Active » Needs work
StatusFileSize
new 3123348-3.patch539 bytes

I'm just adding image style here with effect Scale, based only on width property. But second point of description is not clear for me in terms of how to do that:
2) Configure the field to use this image style

So we have to apply somehow our image style to the Image field of CT Article Default display. Some thoughts:

We don't know if installation profile contains CT "Article".

That means:
- We can't just copy/paste core.entity_view_display.node.article.default.yml config from for example Standard install profile to olivero/config/install and just override Image style of Image field there.
- We can't copy/paste all article's configs into olivero/config/install since user may not need an Article CT. For example Minimal standard profile doesn't have any CTs.

We may somehow override CT Article Default display from theme?

But i didnt find any possible solutions to do that. The principle was like:
- I use some hook to apply my changes right after Olivero got installed.
- I'm checking using \Drupal::configFactory() if Article's configs exist and then i'm trying to apply our image style to the Image field.

From what i found is that hook, but it didnt work for me. I was testing it like:

function olivero_themes_installed($theme_list) { }

So any ideas about all of it?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I also think that it's bad idea to provide config for image style inside of theme, and more weird when theme expects presence of image style or article teaser.

Instead better to create template override for image field from standard profile so it will be used when theme become default in core

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

@andypost I was hoping to keep the scope within the theme... but I agree that you're right that we should modify the standard profile.

@kostyashupenko is it possible to invoke the new image style via preprocess, and call that variable from the template? We're still theming against 8.x for now. We can create an issue to modify the 9.1 standard profile when we submit the core patch. Thoughts?

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
StatusFileSize
new 3123348-6.patch1.51 KB

I don't think my patch is usable and i don't think we should use it somewhere. Main problem as i described already in my previous comment here is the following:
- Olivero theme doesn't know anything about installation profiles. Such things should live in installation profiles, 100% sure.

And some issues:

  1. i don't know the way to create new image style in olivero.theme file. I don't know if it is actually possible. I'm not backender, but still, absolutely all really strong backenders in drupal runs away when i'm trying to ask about good solution for this issue. And this is normal reaction, because such things should live in core's profiles, not in theme.
  2. About new issue for "standard" profile of d9 core - i'm completely agree we need it. Since Olivero is supposed to be a base theme in d9 - then 100%. At least we should have much more than 3 image styles in "standard" profile. Since width of article's image for "Olivero" is supposed to be 1090px - i don't see any problems if we'll add that image style to "standard" profile.
  3. About d8 solution - it's still opened for discussions, need to share maybe that task with more amount of backenders. But maybe we can update "standard" profile too for d8? Like add 1 or 2 new wide image styles.
kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

This is awesome. There were a few minor things that I'm going to fix (see below).

I'm going to rename the image style to olivero_hero, and have the visible name be Hero. 

  1. +++ b/config/install/image.style.olivero_banner.yml
@@ -0,0 +1,14 @@
+name: oliver_banner

    The name of this doesn't match the yml file name. 

  2. +++ b/olivero.theme
@@ -169,3 +169,13 @@ function olivero_preprocess_menu_local_task(&$variables) {
+    $variables['items'][$key]['content']['#image_style'] = 'banner';

    This needs to match the machine code of the image style.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3123348-8-image-style.patch1.5 KB

Re-rolled and updated patch attached.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
StatusFileSize
new 3123348-10-image-style.patch1.5 KB

One more patch... this time with upscaling turned off.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Created followup issue #3124297: Create Drupal 9 core issue to add a "hero" image style and let article use this by default for the full view mode. to fix this in 9.x when we submit a core patch.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Fixed

Committed! Thanks @kostyashupenko

Status: Fixed » Closed (fixed)

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