#2980029: Improve Umami demo's support for managing field display settings proposes changes to Demo Umami's Article and Recipe content type full page view modes in order to support a user making changes to the Manage Display settings and for these changes to be reflected in the theme. We want the Umami demo to provide an intuitive experience for anyone who experiments with display settings.

Tasks:

  • Create a patch that provides the necessary changes to Demo Umami's configuration and the Umami theme to reflect the designs in the parent issue
  • Patch review and testing
  • x-browser testing
  • Accessibility testing
  • Testing for how well the alterations support the types of field display changes a user exploring Drupal might make in the Manage Display settings on Article and Recipe content types
CommentFileSizeAuthor
#24 interdiff_17-24.txt2.29 KBkjay
#24 drupal_core-umami-manage-display-article-recipe-2983576-24.patch32.17 KBkjay
#18 drupal_core-umami-manage-display-article-recipe-2983576-17.patch32.19 KBkjay
#14 interdiff_3-14.txt10.09 KBkjay
#14 drupal_core-umami-manage-display-article-recipe-2983576-14.patch32.15 KBkjay
#9 Victoria_sponge_cake___D8.png2 MBmohit_aghera
#5 Screenshot-2018-7-6 Crema catalana Site-Install.png1.32 MBkjay
#5 Screenshot-2018-7-6 Skip the spirits with delicious mocktails Site-Install.jpg943.52 KBkjay
#4 Screen Shot 2018-07-06 at 16.05.24.png912.06 KBjohn cook
#4 Screen Shot 2018-07-06 at 16.05.53.png981.46 KBjohn cook
#4 Screen Shot 2018-07-06 at 16.05.11.png713.46 KBjohn cook
#4 Screen Shot 2018-07-06 at 16.01.12.png43.98 KBjohn cook
#4 Screen Shot 2018-07-06 at 16.01.02.png75.72 KBjohn cook
#4 Screen Shot 2018-07-06 at 15.55.05.png138.26 KBjohn cook
#4 Screen Shot 2018-07-06 at 15.54.50.png341.12 KBjohn cook
#3 drupal_core-umami-manage-display-article-recipe-2983576-3.patch28.07 KBkjay

Comments

kjay created an issue. See original summary.

kjay’s picture

kjay commented

Confirming that I am well under way with a patch for this and will post back for review soon.

kjay’s picture

kjay commented
Assigned: kjay » Unassigned
Status: Active » Needs review
StatusFileSize
new drupal_core-umami-manage-display-article-recipe-2983576-3.patch28.07 KB

Based on the functional and design changes suggested in the parent issue #2980029: Improve Umami demo's support for managing field display settings, here's a patch that covers the config and theme changes that apply specifically to the Article and Recipe content types.

Below is a list of the things changed by this patch. Note that the only deviation I have intentionally made from the designs is that the recipe page looks better with ingredients columns extending across the full body area width, as do the headings for ingredients and recipe instructions.

With this patch it should now be possible to use the Manage Display settings effectively for Article and Recipe content types and for the output of the corresponding full page view mode to make sense. That's not to say that our theme covers the theming for other field types that a user may choose to add, we remain limited to our focus on the fields required for the Umami content model and any fields that happen to work fine when added.

Changes made:

Recipe content type:

  • Adjusted the order and settings for fields on the Default and Full content View modes
  • Reworked the styles for the Recipe content type to follow the much simpler design
  • Chose to override Classy’s use of floats for inline fields, switched this to flexbox to avoid float clearing issues
  • Adding a background colour/pattern to the image field as well as limiting the image width to 796px (our current full image resolution)
  • Introduced new .css files for fields being re-themed and I believe this follows more of a component styles structure
  • Opted for using css columns for recipes. There is an issue here in that some recipes have used field item entries to add sub-headings eg: 'For the filling' to group the flat list of ingredients. This is a reason for using css columns as opposed to flexbox rows or floats in that they read vertically down the page before splitting across to the next column. I would imagine this is the natural way users will read a list, especially with the larger gutter (it could even be larger). Feedback on this is going to be important I think

Article content type:

  • Adjusted all the fields for Default and Full View modes
  • Removed styles no longer necessary for theming this page in such a 'designed' manner
john cook’s picture

john cook commented
Issue summary: View changes
Status: Needs review » Needs work
StatusFileSize
new Screen Shot 2018-07-06 at 15.54.50.png341.12 KB
new Screen Shot 2018-07-06 at 15.55.05.png138.26 KB
new Screen Shot 2018-07-06 at 16.01.02.png75.72 KB
new Screen Shot 2018-07-06 at 16.01.12.png43.98 KB
new Screen Shot 2018-07-06 at 16.05.11.png713.46 KB
new Screen Shot 2018-07-06 at 16.05.53.png981.46 KB
new Screen Shot 2018-07-06 at 16.05.24.png912.06 KB

I've looked through the full content for articles and recipes. It looks nice and lends itself to show off other features in the future (eg field layout).

I have found some differences between the mock-ups and the rendered pages.

Recipie

Mock-up Page on site

In the mock-up, the ingredients and method sections appear to have a max width (796px?), where on the page they are full width.

The ingredients section also has the checked background – same as the image.

Articles

Mock-up Page on site

The blockquote is styled slightly differently – left-aligned and with a max width in the mock-up; centered and full-width (with padding) on the page.

The inline image (with caption) also has small differences. On the mock-up the image is full width with the caption in a lighter color. The page the images are not full width and, with a caption, gain extra spacing. The caption is the default text color.

Setting to needs work so that these differences can be checked by the designer.

kjay’s picture

kjay commented
StatusFileSize
new Screenshot-2018-7-6 Skip the spirits with delicious mocktails Site-Install.jpg943.52 KB
new Screenshot-2018-7-6 Crema catalana Site-Install.png1.32 MB

To help with review. Here's screenshots of each of the content types after the patch in #3 is applied.

Article screenshot

Recipe screenshot

@John Cook, thanks for reviewing:

Ingredients list:

You can see from these screenshots that I had intentionally deviated from my design for these changes on the Recipe content type to not have the Ingredients list width limited. I'm a little torn on which looks better and it's probably splitting hairs. I did think that given the ingredients list gets reasonably long on some recipes, more space for content on larger screens is of some benefit.

To achieve the designed version, we will need a field-specific twig file for that field in order to add an additional div wrapper for the background effect. I'll experiment with that and update here shortly. Feedback on whether sticking with the ingredients layout as per this patch very welcome!

Blockquote and Inline Images

I need to check but these two elements should probably be spun off as their own issues. The designs have not had the body text updated since version 1 as I've been focusing just on layout for this issue. For example, the originally proposed light grey text for image captions won't meet the accessibility goals we have.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

This should also help with content moderation support, as there is currently no way to display the content moderation bar on the page for recipes for example.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kjay’s picture

kjay commented
Status: Needs work » Needs review

Setting back to Needs Review as I think we probably should go with the version in this patch that sets the list of ingredients to use the container width for its content. I'll try and get to updating the designs to avoid confusion.

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new Victoria_sponge_cake___D8.png2 MB

@kjay
I applied this patch on 8.7.x as well and applied properly.
Implementation approach and related things looks good.
I noticed one styling issue on recipe landing page. I think it is caused due to recent styling change.
I have attached in this screenshot. Recipe detail page

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@mohit_aghera: that is actually by design. See

markconroy’s picture

markconroy commented
Status: Needs review » Needs work

@kjay

The 'Manage Display' tab for full view mode does not work for articles, because `{{ content.field_tags }}` is hardcoded still. That needs to be removed from node--article--full.html.twig and also remove the `|without(field_tags)` filter from the main printing of `{{ content }}`

The 'Manage Display' tab for full view mode for recipes works a treat. Well done.

For the CSS, I think we should run it through the official Drupal comb - https://www.drupal.org/docs/develop/standards/css/csscomb-settings-for-d... - just to make sure we have everything in the right order.

If we are going to add a `.by-author` class to the node--article--full and the node--recipe--full templates, we should probably also include a node--full template, so if any new content types are created by users and they leave the author info there, they can benefit from this. (or maybe leave it and they'll get default settings).

.node--type-recipe.node--view-mode-full .field--name-field-image img {
  max-width: 100%;
  display: block;
}

We should not need the max-width property here, as we already have it on the img element in base.css

.field--label-inline.field {
  display: flex;
  flex-wrap: wrap;
}

.field--label-inline .field__label,
.field--label-inline .field__items {
  /* Undo use of floats by classy, switch to flexbox */
  float: none;
}

^ I quite like that :-)

In `ingredients.css` we have some items set as `rem` and some as `em`. We should probably standardize these. Probably set them all to rem. We might need to do that over more files than just your changes.

Also, if we are not using this, let's remove it:

@media screen and (min-width: 30rem) { /* 480px */
  .field--name-field-ingredients .field__item {
    /* margin: 0 4em 0.6em 0; */
  }
}

Besides that, the patch looks really good.

I haven't looked at the config*yml files, as they are just what Drupal pops out.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

I looked at the config yamls before and they looked just fine.

kjay’s picture

kjay commented
Assigned: Unassigned » kjay

Thanks for the reviews. Assigning back to myself to take a look through @markconroy's suggestions

kjay’s picture

kjay commented
StatusFileSize
new drupal_core-umami-manage-display-article-recipe-2983576-14.patch32.15 KB
new interdiff_3-14.txt10.09 KB

New patch and interdiff attached addressing the issues noted in #11.

Mix of em's and rem's in ingredients.css has been adjusted and the mix should now be correct based on attributes relating to the font size.

Please note that I found an additional issue with the node.html.twig where 'display_submitted' is included twice, and it was adopting 'Classy' theme's approach to the html of wrapping with the element immediately after the element. I've therefore removed the duplication in node.html.twig and for consistency adopted the same HTML format in node--recipe--full.html.twig and node--article--full.html.

The CSS included in this patch have been run through CSSComb as per @markconroy's suggestion.

kjay’s picture

kjay commented
Assigned: kjay » Unassigned
Status: Needs work » Needs review

Oops. Needs Review. Unassigning myself.

Status: Needs review » Needs work

The last submitted patch, 14: drupal_core-umami-manage-display-article-recipe-2983576-14.patch, failed testing. View results

markconroy’s picture

markconroy commented

Can we make sure @smaz gets a credit on this please?

kjay’s picture

kjay commented
StatusFileSize
new drupal_core-umami-manage-display-article-recipe-2983576-17.patch32.19 KB

New patch now with the latest 8.6.x changes merged in which should fix test failures.

kjay’s picture

kjay commented
Status: Needs work » Needs review

Setting to needs review

markconroy’s picture

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

I'm movin' this to R2D2 RTBC

Gábor Hojtsy credited smaz.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Attempted to commit this because IMHO it looks great. However.

  1. $ git apply --index drupal_core-umami-manage-display-article-recipe-2983576-17_0.patch 
drupal_core-umami-manage-display-article-recipe-2983576-17_0.patch:768: trailing whitespace.
?PNG
warning: 1 line adds whitespace errors.

    I did not exactly understand why the PNG needed to change, especially that it introduces a whitespace error :)

  2. When attempting to commit, our scripts run stylelint which was not happy and consequently did not let me commit it of course:
    core/profiles/demo_umami/themes/umami/css/components/fields/ingredients.css
 54:3  ✖  Unexpected empty line before declaration   declaration-empty-line-before
  3. The CSS docblocks should be sentences at the start of the CSS files. Missing periods at the end.
kjay’s picture

kjay commented
Status: Needs work » Needs review
StatusFileSize
new drupal_core-umami-manage-display-article-recipe-2983576-24.patch32.17 KB
new interdiff_17-24.txt2.29 KB

New patch addressing issues raised by @Gábor Hojtsy

Regarding #23 1) This is a new PNG for the background pattern introduced for the image on Recipe content type. I believe the white spaces are unavoidable when they crop up in images?

Cheers!

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

The changes look good. The whitespace in images is unavoidable, it's popped up in several different patches that added content images.

  • Gábor Hojtsy committed 9fb88f6 on 8.7.x 
    Issue #2983576 by kjay, John Cook, mohit_aghera, markconroy, smaz:...

  • Gábor Hojtsy committed 9949db9 on 8.6.x 
    Issue #2983576 by kjay, John Cook, mohit_aghera, markconroy, smaz:...
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for resolving the little issues I found.

Committed 9949db9 and pushed to 8.7.x and 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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