The original design for the recipe content type in Umami used a complex field layout that was not possible without Layout Builder being stable. Umami began making use of Layout Builder to manage the display of the recipe content type's full page view mode in 8.7.x but this implementation was a simple demonstration of the type's fields using one column Layout Builder sections.

This issue follows up on that work to use Layout Builder to display the recipe content type in a way that is more closely aligned to the original design (#2900720).

Umami original recipe content type design.

I propose a few interpretations of this original design layout in order to keep the demo flexible with how core works:

  1. Stack the recipe category and tags fields, don't 'inline' them like they are in this visual.
  2. Move the summary to appear above the image and extend 100% width of its container. This will be more practical, flexible, and is probably a better design approach.
  3. Remove the cream coloured background section from behind the ingredients and method. This will require layout styles and may be a future enhancement to demonstrate such a feature. But will simplify the implementation.
  4. Remove the "What you'll need and how to make this dish" sub-heading to ingredients and method. This would require complexity in the master layout and isn't necessary for the demo experience.

Note:

Although not essential, I recommend this issue should be dependent on the responsive layout style improvements made in #3001660. As such, any patches supplied on this issue would ideally be visually tested with that issue applied also.

CommentFileSizeAuthor
#25 interdiff.txt7.2 KBlauriii
#18 Screen Shot 2019-11-21 at 20.58.59.png74.2 KBlauriii
#18 Screen Shot 2019-11-21 at 20.58.48.png53.07 KBlauriii
#14 interdiff-11-14.txt2.85 KBkjay
#14 drupal-umami_recipe-ctype-layout-builder--3085534-14.patch26.79 KBkjay
#13 Screen Shot 2019-11-06 at 11.30.55.png208.12 KBlauriii
#11 interdiff_10-11.txt1.23 KBshaal
#11 drupal-umami_recipe-ctype-layout-builder--3085534-11.patch26.72 KBshaal
#10 reroll_diff_5-10.txt12.04 KBshaal
#10 drupal-umami_recipe-ctype-layout-builder--3085534-10.patch26.08 KBshaal
#8 recipe-layout-builder.png1.56 MBwebchick
#5 interdiff-2-5.txt3.25 KBkjay
#5 drupal-umami_recipe-ctype-layout-builder--3085534-5.patch25.47 KBkjay
#2 recipe-lb-2-3-4-columns.png883.7 KBkjay
#2 recipe-lb-small.png946.18 KBkjay
#2 recipe-lb-desktop.png762.48 KBkjay
#2 drupal-umami_recipe-ctype-layout-builder--3085534-2.patch25.53 KBkjay
Umami - Recipe - v1 - Desktop.jpg1.51 MBkjay

Comments

kjay created an issue. See original summary.

kjay’s picture

kjay commented
StatusFileSize
new drupal-umami_recipe-ctype-layout-builder--3085534-2.patch25.53 KB
new recipe-lb-desktop.png762.48 KB
new recipe-lb-small.png946.18 KB
new recipe-lb-2-3-4-columns.png883.7 KB

Here's a patch that provides the following changes:

  1. Add a new Layout Builder layout called 'One plus four grid' to deliver the layout for the band in the design where the image and 4 x recipe meta icons are.
  2. Override core's Layout Builder library implementations for one, two, three, and four column layouts so that Umami provides exactly the same layout features but adds gutters to these layouts as default. Content now added to any Layout Builder layout will not crash its neighbour column. This change was only required for the two column layout which is used for the Ingredients and Method field but I believe it makes sense to bundle all four options into this patch for consistency.
  3. Adjust the four column layout so that it drops to two columns between Medium to Large breakpoints.
  4. Adjust specific components margins so that their presentation is more flexible in a Layout Builder 'can be placed anywhere' world.
  5. Note: I have been developing this with the work in #3001660 applied first. Aside from visual layout enhancements, this related issue adjusts the responsive behaviour of images.
  6. Adjust install config to make use of the new regions and position the fields as per the issue summary description.

How it looks at desktop size:

Recipe with layout builder enhanced - desktop

How it looks at small size:

Recipe with layout builder enhanced - small

How default layouts with gutters now look with 2, 3, 4 columns

Recipe with layout builder enhanced - 2, 3, 4 columns with gutters.

kjay’s picture

kjay commented
Status: Active » Needs review

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

kjay’s picture

kjay commented
StatusFileSize
new drupal-umami_recipe-ctype-layout-builder--3085534-5.patch25.47 KB
new interdiff-2-5.txt3.25 KB

We reviewed this issue in the Out of the Box call this week and @markconroy noted that it would be better to follow the same method for layout in the 2 column and one plus four grid layout as we have in the 3 column where negative margins are used on the wrapper and equal left/right margins are applied to the children.

I also found errors in my original layout work so here is a new patch for review.

markconroy credited shaal.

markconroy’s picture

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

We reviewed this on a number of calls this week over Google Hangouts.

Marking RTBC, reviewed by myself @shaal

Thanks for the work on this @kjay

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
StatusFileSize
new recipe-layout-builder.png1.56 MB

LOOK AT THAT!! IT'S GLORIOUS!!

I'm not sure I feel comfortable reviewing/committing this myself, since there's a lot of front-end code in there, but HOLY COW does that look great!! :D

Recipe page layout shown in Layout Builder.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs a reroll 🧚‍♂️

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll +Out of the Box Initiative
StatusFileSize
new drupal-umami_recipe-ctype-layout-builder--3085534-10.patch26.08 KB
new reroll_diff_5-10.txt12.04 KB

After media images got committed to core, attempting a re-roll... going once...
(and adding a reroll diff instead of the regular interdiff)

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented
StatusFileSize
new drupal-umami_recipe-ctype-layout-builder--3085534-11.patch26.72 KB
new interdiff_10-11.txt1.23 KB

Found a CSS code in recipe.css that was removed in #5 but I restored it by mistake, removing that CSS again.

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented
Status: Needs review » Reviewed & tested by the community

I ran a full visual regression test comparison through https://diffy.website

When applying the latest patch from #3001660: Fix Umami's responsive layout styles + the latest patch from this issue, the result is stunning.

I compared the previous RTBC'd patch and the current patch, you can download the visual comparison differences (150MB).
You'll notice that the only changes are actually unrelated (improvements in Drupal core's translation to Spanish...)
https://diffy-files.s3.amazonaws.com/2019/11/06/Umami88x2019-11-6-1903.t...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new Screen Shot 2019-11-06 at 11.30.55.png208.12 KB

  1. This could be a follow-up or something we don't want to do but I noticed that the category and tags are inline on the designs. Has that changed since then or should we make that change to make the designs match more closely with the original designs? 

  2. +++ b/core/profiles/demo_umami/themes/umami/layouts/oneplusfourgrid_section/layout--oneplusfourgrid-section.html.twig
@@ -0,0 +1,61 @@
+      <div class="grid-wrapper">

    Maybe this could be more specific. grid-wrapper sounds very generic. Something like layout__four-grid-group would at least in my opinion be clearer. 

  3. +++ b/core/profiles/demo_umami/config/install/core.entity_view_display.node.recipe.full.yml
@@ -59,43 +62,82 @@ third_party_settings:
+        layout_id: layout_gridright__section

+++ b/core/profiles/demo_umami/themes/umami/umami.layouts.yml
@@ -0,0 +1,21 @@
+layout_gridright__section:
+  label: 'One plus four grid'
+  path: layouts/oneplusfourgrid_section
+  template: layout--oneplusfourgrid-section
+  library: umami/oneplusfourgrid_section

    Should we make the machine name match with these? Also, there's a double underscore before the section.

kjay’s picture

kjay commented
Status: Needs work » Needs review
StatusFileSize
new drupal-umami_recipe-ctype-layout-builder--3085534-14.patch26.79 KB
new interdiff-11-14.txt2.85 KB

Thank you @lauriii for your review. I have addressed the points below and in the attached patch.

  1. I agree that it would look better to inline the fields but with the goal being to support fields being placed arbitrarily via Layout Builder, I'm not too sure how we might approach targeting these fields specifically without their layout being affected if positioned elsewhere? Agreed that if we do follow up on this then a follow-up issue makes sense.
  2. I have revised the patch to use your class name recommendation.
  3. Yes this was an oversight of mine, thanks for spotting it.
shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented
Status: Needs review » Reviewed & tested by the community

Interdiff file looks good!
I finished running visual regression test with diffy.website between patch #14 and the previous patch.
0 differences, and all pages look exactly as they should!
RTBC'd :)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

The changes in #14 address all of my feedback on #13. We're on a commit freeze right but I will come back here after the freeze ❄✌️

shaal’s picture

shaal
he/him
Primary language English
Location Boca Raton, FL
commented

You can use this link to view the Diffy visual comparison of before/after this patch + #3001660: Fix Umami's responsive layout styles

https://app.diffy.website/#/shared/MTgxMTExNDA2/diffs/18111

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue summary: View changes
StatusFileSize
new Screen Shot 2019-11-21 at 20.58.48.png53.07 KB
new Screen Shot 2019-11-21 at 20.58.59.png74.2 KB

Do we want the icons to be present in Layout Builder?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work

Moving back to needs work after talking to @shaal and @kjay.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Needs followup

#18 is a pre-existing issue not caused by this. Let's open a follow-up for that. Feel free to move this back to RTBC once the issue has been opened 😇

markconroy’s picture

markconroy commented
Status: Needs work » Reviewed & tested by the community

@lauriii follow-up created here #3096350: Layout Builder omits view-mode class when editing layout

Setting back to RTBC

  • lauriii committed f193ad9 on 9.0.x 
    Issue #3085534 by kjay, shaal, lauriii, webchick, markconroy: Configure...

  • lauriii committed df7752a on 8.9.x 
    Issue #3085534 by kjay, shaal, lauriii, webchick, markconroy: Configure...

  • lauriii committed 970e9e4 on 8.8.x 
    Issue #3085534 by kjay, shaal, lauriii, webchick, markconroy: Configure...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup
StatusFileSize
new interdiff.txt7.2 KB

Thank you for opening the follow-up!

I applied yarn run lint:css --fix and fixed manually a coding standard problem in the Twig template. Interdiff attached.

Committed f193ad9 and pushed to 9.0.x, 8.9.x and 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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

chenx319’s picture

chenx319 commented

hello , is this feature is released in drupal 8.9.13 ?

markconroy’s picture

markconroy commented

Hi @chenx319

Yes, this feature has been in Drupal core since 8.8 which was released about a year ago.