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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks’s picture

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

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

Et voilà ;-)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +Needs tests

Sorry.

attiks’s picture

attiks CreditAttribution: attiks commented

#3 ++

Jelle_S’s picture

Jelle_S
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: Jelle_S commented
FileSize
2424727-5-resp-img-no-empty-media-attr.patch4.75 KB
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 CreditAttribution: 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
CreditAttribution: Jelle_S commented
FileSize
2424727-7-resp-img-no-empty-media-attr.patch4.75 KB
interdiff-5-7.txt735 bytes
  1. yes
  2. no, that's the property name on the entire class
attiks’s picture

attiks CreditAttribution: 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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
CreditAttribution: Jelle_S commented
Status: Needs work » Needs review
FileSize
2424727-11-resp-img-no-empty-media-attr.patch4.82 KB
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 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

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

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

alexpott’s picture

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

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

attiks’s picture

attiks CreditAttribution: attiks commented

#16 It is, mainly for the mobile breakpoint

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers 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
CreditAttribution: RainbowArray 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 🇪🇺🌍
CreditAttribution: alexpott 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.