Umami - hero block is identical on Home and Recipe pages, so potentially confusing.

You can configure block and remove "/recipes" from pages visibility settings.

/admin/structure/block/manage/umami_banner_recipes

OR you can update block.block.umami_banner_recipes.yml file on line No. 31 with (pages: "<front>") before installation.

File directory is core\profiles\demo_umami\config\optional

Visibility is set to "front" page for umami_banner_recipes

the above code needs to be reviewed

This is a good point and the hero banner on home is there because the front page is not yet complete. But I would propose that we could also improve this by keeping both banners and introducing new hero banner content for the recipes landing page. I am currently working on finding a suitable image that can be used for linking to one of the recipes. I will update shortly.

We could use the attached hero banner image as a second hero banner block on the recipes landing page and link to the existing Vegan Chocolate Brownie recipe. However, this hero image relies on a related issue to update the Vegan Chocolate Brownie Recipe #2953995: Update the Umami Vegan Chocolate Brownie recipe that we need to get in at the same time for 8.7.

We can use the title and summary text of the linked recipe to overlay to the left in white text.

Attached also is a guide mockup for how this should end up looking.

Working on this now as part of Global Contribution Weekend, in Leeds, UK

Okay - plan:

1. Take Akshaya's patch at #3 forward but renaming the block machine name to "umami_banner_front"
2. Add a new block with the new recipe image in it using the block machine name "umami_banner_recipes"
3. Add appropriate content

Okay, here we go...

NB this is blocked on #2953995: Update the Umami Vegan Chocolate Brownie recipe

I am officially daft and forgot to run tests after changing the content!

Updating now...

The test needed updating as there are now two banner blocks and the one on the recipe page is a new one, so has a new UUID...

Would be good to update the test that checks the block placement for the new block on the Recipe page too.

on it...

Okay - made a start on this but *not working yet*. I need to get on the bike home before it gets dark (if the sun get too low, I don't want to be too close to any errant Dukes...)

Picking this up from where rachel_norfolk left off.

Just adding a test run on my "non working" patch so there is canonical description of it not working.

Hi Rachel, glad to hear you didn't have a run in with Phil back in Norfolk!

Update on where I'm up to: the blocks are working fine, that all looks good and then onto testing. Took a while to set up to get the tests (any test) to pass on my environment due to a PHP7 issue and now working through syntax. There is some work to do around line 50 where it is currently breaking but I don't think there is going to be much left to do thanks to your efforts on this.

Ooooooooh - just realised something! I changed the data structure a bit in expectedBlocks() and then changed how it was used a little in assertImportedCustomBlocks() all week and good.

Thing is, expectedBlocks() is also called at line 48 in testReinstall(). That needs adapting in a similar way to assertImportedCustomBlocks() to match the changed data.

Yes Rachel that’s it. Hopefully when that’s in we’ll get a pass and if not will strip out bits to troubleshoot.

Removed tag novice.

We are working on UninstallDefaultContentTest.php, documenting lines I have isolated:

    foreach ($this->expectedBlocks() as $expected_blocks) {
                  foreach ($expected_blocks['block_info'] as $block_info) {

at line 48 and closing bracket seems to be correct.

$block = $block_storage->loadByProperties(['uuid' => $block_info['uuid']]);

is causing a problem in the two places it appears. $block_info['uuid'] probably needs expanding.

I think there may be something else with the assertImportedCustomBlocks() but if there is it will be hard to trace from errors due to it breaking from where the function is called.

Consider renaming UninstallDefaultContentTest.php it appears the tests have expanded since the file got its name.

Got this to pass, patch to come.

Went back to the 8.7.x version of this file and added path variable to the array so we are checking disclaimer and promo blocks only on the recipes page along with recipes banner block (as before) but now we're checking for the new block on the front page too. With this approach we do not check those blocks on both pages but we avoid problems with other tests cycling through the same block twice.

Tested, it looks great
(and delicious!)

Hi Shaal - thanks!

If you could detail how you texted it (and that is probably very obvious) and show a little screenshot or two showing where the recipe appears and that the recipe index shows the new image at the top, then that would probably count as an appropriate level of testing evidence to take this forward to RTBC. Would love that extra detail on the testing to get you a credit on this issue!

Hi Rachel.
Oops. Sorry about that, we discussed it further during the OOTB conference call, I'll add all the details here -

There are 3 items in the main menu Hero, Articles and Recipe

There's a new hero image for /recipes page

Compared to the existing hero image on the homepage -

We discussed that a new image will be designated to a new hero on /articles page to keep consistency.
A new issue will be created for that.
Here's the current /articles page -

But when I tested it on RTL, there was a major accessibility issue, since the bright color text is placed on top of food texture
Here's the problem on homepage RTL -

Here's the problem on /recipes RTL -

Solutions we discussed were:

• Adding a partially transparent background behind the hero's text, will work for both LTR and RTL
• RTL version - move the hero's text to the other side of the screen, so it will be placed on darker bakcground
• Add configuration for editors to choose between bright image / dark image, and create an alternative version text on top of bright image

There's another minor related issue -
The /recipes hero headline is "Vegan chocolate brownies",
but the title of the recipe that it is linked to is "Vegan chocolate and nut brownies"

Hmm - I wonder if the most obvious solution, given we know the images contain no text, is simply to flip them in css?

I don't think flipping the background image hero will provide a good solution to this problem.
Flipping background images will manipulate the content. When users test out OOTB on RTL and upload their own images they might get unexpected results if their images contain text.

Can we please keep the conversation about dealing with text contrast ratio in banner images in #2960795: Text fails contrast in banner please? This is a known problem that is being discussed, is not exacerbated particularly by this issue and should not stop this issue from moving forward.

I have spun this up locally with quick start and it looks great.

Couple of points. Firstly I'm not sure why we're referring to this as the Front banner rather than the Home banner. We mostly refer to this page as the Home page rather than the Front page.

Secondly, we have some coding standards issues we need to fix:

InstallHelper.php last indentation in this block in importBlockContent() is two spaces too much

          'uri' => 'internal:' . call_user_func(function () {
              $nodes = $this->entityTypeManager->getStorage('node')->loadByProperties(['title' => 'Vegan chocolate and nut brownies']);
              $node = reset($nodes);
              return $this->aliasManager->getAliasByPath('/node/' . $node->id());
            }),

UninstallDefaultContentTest.php has duplicate newlines after assertImportedCustomBlock() and at the end of the file.

a little lunchtime distraction...

think I've got everything in #35 and the final comment in #31.

Definitely getting there!

Think we've been over-zealous on the whitespace at the bottom of the UninstallDefaultContentTest class. There is supposed to be a blank line between the closing brace of the expectedBlocks() function and the closing brace of the class.

Also the link in the new block is in uppercase

Whereas the corresponding block on the home page is not

I'm also not sure why one uses the recipe title and the other says "view recipe". The recipe title actually seems redundant in the link so view recipe
makes more sense. One for @kjay I suspect.

I think 'View recipe' is the better option without uppercase, but if keeping uppercase is preferred it should move to CSS.

I'll action #37 at the weekend if no one gets there first.

I think keeping the uppercase is unlikely given #2958239: Readability problem with all-caps text in core themes

I just followed the mock-up in #9.

'View recipe' seems like a good idea to me (without the uppercase).

I had always hoped we could return to the buttons being labelled 'View recipe', 'View article', etc, as per the designs. I am not sure what technical reason meant we ended up with the title being repeated which is makes the button so big, I've always been tempted to suggest we move away from a button if it's an essential it contains so much text!

I wonder if this was an accessibility decision?

Oops - sorted out my phpcs now!

1. Corrected code style issues.
2. Changed BOTH custom block links to read "View recipe"

I have spun this up again and confirmed it does as intended. The new image looks fantastic and I think the change to use View recipe on the buttons rather than repeating the title is the right one. Reviewing the patch shows the coding standards issues are now addressed.

Moving to RTBC.

Win sauce.

This looks great, really good to see some of the new images dropping into place and I think it works really well on this hero banner. Except...

I'm really sorry but I managed to miss a fudge on my photoshop work that really shows on my larger screen. There's still a hard edit line where I've joined the left side painted bits onto the left edge of the image. Should be fixed in the attached patch. Sorry for that!

I've reviewed the interdiff and confirmed only the image has changed. Installing Umami and the glitch mentioned in #44 and confirmed with @kjay in Slack is now not present.

Returning to RTBC.

Fail looks unrelated. Putting back to Needs Review to retest.

@Eli-T not clear why set back to RTBC after #47. Does this need further review and testing?

@tonypaulbarker the patch in #44 looked good but the testbot failed. Looking at the testbot results, it looked like a failure that was unrelated to the changes we made, so I set the status back to Needs Review. This causes the testbot to try again without having to re-upload the patch. Once that test run was complete and successful, I set it back to RTBC as it had been reviewed, and both manually and automatically tested.

No further action should be required, unless the committers deem further changes are necessary.

@Eli-T, gotcha. Thanks!

Please update issue summary with what is actually happening. Looks like the patch does not implement what the summary proposes.

Updated IS - please review it now describes change more clearly.

The summary looks good Rachel.

Corrected typo in the username of the original reporter.

After reading back, have also changed 'language' to 'text' to remove ambiguity under 'Proposed resolution' since this patch does not change multilingual content.

In that case, I believe we can set it back to RTBC...

+1 for RTBC based on the new description.

Superb, thanks all!

Not that it matters a whole lot to me but what makes #2953995: Update the Umami Vegan Chocolate Brownie recipe an umami content patch that gets back ported and this one not? There's only content changes here, right? (assuming block content and the placement of blocks is counted as content)

Just trying to learn more about process.

Yeah this could go back to 8.6.x indeed. Thanks for calling that out.