Follow-up to #2424587: Make the mobile breakpoint for Bartik empty

Problem/Motivation

I set up a responsive image style using thumbnail for the mobile breakpoint (which has an empty mediaQuery). When I set article image to use the responsive image style and then created an article with a test image, the source element for the mobile breakpoint had an empty media attribute.

CommentFileSizeAuthor
#11 interdiff-7-11.txt1.64 KBjelle_s
#11 2424727-11-resp-img-no-empty-media-attr.patch4.82 KBjelle_s
#7 interdiff-5-7.txt735 bytesjelle_s
#7 2424727-7-resp-img-no-empty-media-attr.patch4.75 KBjelle_s
#5 interdiff-2-5.txt3.91 KBjelle_s
#5 2424727-5-resp-img-no-empty-media-attr.patch4.75 KBjelle_s
#2 2424727-resp-img-no-empty-media-attr-2.patch859 bytesjelle_s

Comments

attiks’s picture

attiks commented
Component: Bartik theme » responsive_image.module
jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs review
StatusFileSize
new 2424727-resp-img-no-empty-media-attr-2.patch859 bytes

Et voilà ;-)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs tests

Sorry.

attiks’s picture

attiks commented

#3 ++

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new 2424727-5-resp-img-no-empty-media-attr.patch4.75 KB
new interdiff-2-5.txt3.91 KB

#3 & #4: LOL, yeah, I figured as soon as I posted it, so I already started on it. Here it is with tests included.

attiks’s picture

attiks commented 
  1. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
@@ -308,6 +308,54 @@ public function testResponsiveImageFieldFormattersLinkToNode() {
+   * Tests responsive image formattes on node display with an empty media query.

    typo formattes? 

  2. +++ b/core/modules/responsive_image/src/Tests/ResponsiveImageFieldDisplayTest.php
@@ -308,6 +308,54 @@ public function testResponsiveImageFieldFormattersLinkToNode() {
+    $this->responsiveImgStyle

    typo ImgStyle?

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new 2424727-7-resp-img-no-empty-media-attr.patch4.75 KB
new interdiff-5-7.txt735 bytes
  1. yes
  2. no, that's the property name on the entire class
attiks’s picture

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

1 out of 2 isn't bad ;-)

RTBC is bot is happy

The last submitted patch, 5: 2424727-5-resp-img-no-empty-media-attr.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests

Nit: the $image_uri line is misplaced. Copy/paste error? Let's put it (and its comment) a bit lower, where it logically belongs.

jelle_s’s picture

jelle_s
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new 2424727-11-resp-img-no-empty-media-attr.patch4.82 KB
new interdiff-7-11.txt1.64 KB

Ah yes, that comment was totally irrelevant. It was indeed a copy-paste error.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2424727-11-resp-img-no-empty-media-attr.patch, failed testing.

attiks queued 11: 2424727-11-resp-img-no-empty-media-attr.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Is it realistic for breakpoints to not have a media query?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

Setting to 'needs review' to get an answer to #16

attiks’s picture

attiks commented

#16 It is, mainly for the mobile breakpoint

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

To clarify #18: the "narrowest" breakpoint has an empty media query. IOW: it's the default. Then for each breakpoint that is wider, a media query is set.

Back to RTBC since #18 answers #16.

rainbowarray’s picture

rainbowarray
she/her
Primary language English
Location Twin Cities, Minnesota
commented

To further clarify, an empty media attribute is useful when using the sizes and srcset attribute, because sizes is used for viewport switching of images rather than a media query filling that role.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 097d6d3 and pushed to 8.0.x. Thanks!

  • alexpott committed 097d6d3 on 8.0.x 
    Issue #2424727 by Jelle_S: Do not output empty media attribute for...

Status: Fixed » Closed (fixed)

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