feat(srcset): add srcset directive to inject <source> elements to sup… #366

benbraou · 2017-08-06T12:52:09Z

Support responsive image by introducing srcset.<breakpoint alias> directive.

It Injects a <source> element for every srcset.<breakpoint alias> in the HTML markup of an <img> element contained in a <picture> element.

Closes #81.

benbraou · 2017-08-06T12:54:01Z

@ThomasBurleson can you please review ?

ThomasBurleson · 2017-08-06T16:18:08Z

src/lib/flexbox/api/img-srcset.ts

+  private _inputSourceEltMap: {[input: string]: any} = {};
+
+  /* tslint:disable */
+  @Input('srcset.xs') set srcsetXs(val) {this._cacheInput('srcsetXs', val);}


@benbraou - you will need a selector and setter for [srcset] also... to cache the default, static value. When the responsive breakpoint deactivates , it is possible that fallback static value (which is used to clear the deactivated value) will be used (if no other breakpoints activate). This means you must databind to srcset and simply cache the value.

See example here: https://github.com/angular/flex-layout/blob/master/src/lib/flexbox/api/style.ts#L62-L66

@benbraou - if you want this in Beta.9, we must fix within the next 24 hours.

Bloody hell.... HURRY MAN! 😄

@ThomasBurleson :requested change is done. Thanks a lot for the explanation ^^

ThomasBurleson · 2017-08-08T16:35:18Z

src/lib/flexbox/api/img-srcset.ts

+   * backward compatibility.
+   */
+  protected _injectSourceElements() {
+    if (!this._mqActivation) {


We need to check angular/universal SSR.

let isBrowser = getDom().supportsDOMEvents(); if (!this._mqActivation || !isBrowser) { return; }

Change done. PR will be updated shortly

ThomasBurleson · 2017-08-08T16:36:17Z

src/lib/flexbox/api/img-srcset.ts

+
+  /** Reference to injected source elements to be used when there is a need to update their
+   * attributes. */
+  private _inputSourceEltMap: {[input: string]: any} = {};


Please change name to _registrySourceElements

Change done. PR will be updated shortly

ThomasBurleson · 2017-08-08T16:39:28Z

src/demo-app/app/docs-layout-responsive/responsivePicture.demo.ts

+                        srcset.md="https://dummyimage.com/500x200/76c720/fff.png&text=md"
+                        srcset.sm="https://dummyimage.com/400x200/b925c7/fff.png&text=sm"
+                        srcset.lt-sm="https://dummyimage.com/300x200/c7751e/fff.png&text=lt-sm"
+                        srcset.gt-md="https://dummyimage.com/700x200/258cc7/fff.png&text=gt-md" >


To test that the default is properly used as fallback, let's change srcset.gt-md to:

srcset.gt-lg="https://dummyimage.com/700x200/258cc7/fff.png&text=gt-lg"

Change done. PR will be updated shortly

ThomasBurleson · 2017-08-08T16:40:38Z

Overall this looks very good. Nice work @benbraou. 🙌

It should be noted that this does NOT support IE 11; which has 3.1% global usage currently. See http://caniuse.com/#search=picture.

Should we add to the example usage of device-pixel-ratio?
e.g.

<img 
   srcset="small.jpg 1x, large.jpg 2x"
   srcset.xs="xsmall.jpg 1x, xsmallLg.jpg 2x"
   src="small.jpg"
   alt="A rad wolf" />

benbraou · 2017-08-08T19:39:38Z

@ThomasBurleson Thanks!
All required changes are done.
I have also added the device-pixel-ratio usage example:

<picture>
    <img  style="width:auto;"
     src="https://dummyimage.com/400x200/c7c224/000.png&text=default"
       srcset.md="https://dummyimage.com/500x200/76c720/fff.png&text=md"
       srcset.sm="https://dummyimage.com/400x200/b925c7/fff.png&text=sm"
       srcset.lt-sm="https://dummyimage.com/300x200/c7751e/fff.png&text=lt-sm(1x) 1x,
       https://dummyimage.com/300x200/f0b16e/fff.png&text=lt-sm(2x) 2x,
       https://dummyimage.com/300x200/f6ca9a/fff.png&text=lt-sm(3x) 3x"
       srcset.gt-lg="https://dummyimage.com/700x200/258cc7/fff.png&text=gt-lg" >
</picture>

In the demo, I have though about updating MediaQueryStatus component (or creating a new one) to display the devicePixelRatio window property, but I decided finally to just update the usage example as you requested. (It may be useful to add it in the future).
Regarding your first point, I am aware that IE11 does not <picture> .
I see two possible solutions:

Update the wiki (I am open to help) with this information and suggest to use the picturefill polyfill
If we detect that the bowser does not support <picture> ,we subscribe to media query changes to update the <img> src value when relevant.

Do you have other ideas?

ThomasBurleson · 2017-08-08T20:30:51Z

@benbraou - Since the mediaActivationResponse will already tell use about MediaChange events, I think we should inject detect IE11 and update the imge.src value.

img-srcset.ts

constructor(
    elRef: ElementRef, 
    renderer: Renderer, 
    monitor: MediaMonitor,
    @Inject(DOCUMENT) private _document:any ) {
  super(monitor, elRef, renderer);
}

 ngOnInit() {
   const isIE = !!this._document['documentMode'];
  

   this._listenForMediaQueryChanges('srcset', '', (change:MediaChange) => {
    if (isIE) {
       // IE11 does not support the <picture> element; so responsively change the <img src="">
       // Normally, there is no need to subscribe to mediaQuery changes as it is
       // up to the browser to use the relevant injected <source> element

       this._updateImageSrc(change.mqAlias);
    }   
   });
   
   if ( !isIE ) {
     this._injectSourceElements();
   }
 }

ThomasBurleson · 2017-08-08T20:35:58Z

Any consideration/question:

What happens when the user does NOT wrap the img inside a picture element?

Scenario #1:

<img style="width:auto;"
     src="https://dummyimage.com/400x200/c7c224/000.png&text=default"
     srcset.md="https://dummyimage.com/500x200/76c720/fff.png&text=md"
     srcset.sm="https://dummyimage.com/400x200/b925c7/fff.png&text=sm"
     srcset.lt-sm="https://dummyimage.com/300x200/c7751e/fff.png&text=lt-sm"
     srcset.gt-lg="https://dummyimage.com/700x200/258cc7/fff.png&text=gt-lg" >

In this case ^ (#1), should we auto-wrap the dom element inside a <picture> node?

Scenario #2:

Use tries to use the src property responsively:

<img style="width:auto;"
     src="https://dummyimage.com/400x200/c7c224/000.png&text=default"
     src.md="https://dummyimage.com/500x200/76c720/fff.png&text=md"
     src.sm="https://dummyimage.com/400x200/b925c7/fff.png&text=sm"
     src.lt-sm="https://dummyimage.com/300x200/c7751e/fff.png&text=lt-sm"
     src.gt-lg="https://dummyimage.com/700x200/258cc7/fff.png&text=gt-lg" >

In this case ^ (#2), should we responsively update the src property OR should we convert these
to srcset values?

benbraou · 2017-08-09T05:45:08Z

I went to sleep 😁
@ThomasBurleson Thanks for taking time to explain your considerations.
I will be able to answer today starting from 19:00 GMT (I am going for work now)
For the implementation, I have free time this weekend (unfortunately not before due to my work commitments...)
Sorry, I guess we are targetting v2.0.0-beta.10 then.

ThomasBurleson · 2017-08-09T13:13:21Z

@benbraou - let's make two separate issue to enhance this current PR. These issues can be fixed for Beta.10.

Issue 1: Polyfill support for IE11 (see comment above)
Issue 2: Support Responsive src API (see scenarios above above)

benbraou · 2017-08-09T18:13:24Z

Rebase done. So, no action items remaining on my side regarding this PR.
I ll open the 2 issues to address your considerations and submit the PRs hopefully this weekend

ThomasBurleson · 2017-08-09T19:16:13Z

@benbraou - superb. Thank you. LGTM

ThomasBurleson · 2017-08-09T19:37:30Z

src/lib/flexbox/api/img-srcset.ts

+  @Input('srcset.gt-lg') set srcsetGtLg(val) {this._cacheInput('srcsetGtLg', val);};
+
+  /* tslint:enable */
+  constructor(elRef: ElementRef, renderer: Renderer, monitor: MediaMonitor) {


@benbraou - to account for pending changes in PR #372, please use the Renderer2 instead of Renderer. This should be the last change required before merging. Thx.

Sure. For this PR, I will inject Renderer2 instead of Renderer as you requested.
For the future 2 PRs, I will take the opportunity to replace getDom().insertBefore by Renderer2 methods.

I went ahead and used renderer.insertBefore. Minor changes overall.

const sourceElt = this._renderer.createElement('source'); this._registrySourceElements[bpX.key] = sourceElt; this._renderer.insertBefore(this.parentElement, sourceElt, this.nativeElement); this._renderer.setAttribute(sourceElt, 'media', bpX.mediaQuery); this._renderer.setAttribute(sourceElt, 'srcset', this._queryInput(bpX.key));

instead of

const sourceElt = this._renderer.createElement(this.parentElement, 'source'); this._registrySourceElements[bpX.key] = sourceElt; getDom().insertBefore(this.parentElement, this.nativeElement, sourceElt); this._renderer.setElementAttribute(sourceElt, 'media', bpX.mediaQuery); this._renderer.setElementAttribute(sourceElt, 'srcset', this._queryInput(bpX.key));

Very nice! Love your attention-to-detail. 👍

ThomasBurleson · 2017-08-10T00:52:30Z

@benbraou - also please git pull --rebase origin master again.

benbraou · 2017-08-10T04:56:07Z

changes and rebase done

benbraou · 2017-08-15T09:46:38Z

conflicts solved

ThomasBurleson · 2017-08-15T12:49:52Z

@benbraou - regarding your comment 3 hours ago... did you change any source? What do you mean with the comment ^?

Note that I am adding more tests to your PR before we can merge.

benbraou · 2017-08-15T13:05:03Z

@ThomasBurleson there were conflicts in base.ts and custom-matchers.ts after the merge of #374 preventing my PR from being merged. So, I did a rebase and fixed those 2 conflicts.

ThomasBurleson · 2017-08-15T13:19:22Z

@benbraou - true, I fixed those in my local copy of your PR also. Stand-by for additional tests.

* add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs * fix ImgSrcsetDirective to support usages without `<picture>` parents Closes #366, Fixes #81, Fixes #376.

…ements * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs * fix ImgSrcsetDirective to support usages without `<picture>` parents Closes #366, Fixes #81, Fixes #376.

…ements * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs Closes #366, Fixes #376.

ThomasBurleson · 2017-08-17T22:01:13Z

@benbraou - these changes will not be included in Beta.9: ImgSrcSetDirective needs more work to stabilize on confirm support for all variations of usage.

PR #382 with the ImgSrcDirective will be added but not your ImgSrcsetDirective.

Let's talk offline about this effort.

…ements * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs Closes #366, Fixes #376.

ThomasBurleson · 2017-08-17T22:19:42Z

@benbraou - see https://github.com/angular/flex-layout/tree/thomas/fix-img-srcset for changes to picture demo, img-srcset.ts, and img-srcset.spec.ts

…ements * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs Closes #366, Fixes #376.

* feat(api, img-src): add ImgSrcDirective for responsive API for img elements * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs Closes #366, Fixes #376. * chore(release): fix package.json version

@benbraou

Support responsive image by introducing srcset.<breakpoint alias> directive. When used as <img> attributes where * where the <img> is a child of a <picture> container, this directive will injects a <source> element for every srcset.<breakpoint alias>. * where the <img> is NOT nested in a <picture> element, then the `img.src` property will be responsively updated. > Thanks to @benbraou for his initial PR submission (@see #366) Closes #81.

* add ImgSrcsetDirective * to inject <source> elements to support responsive images * Inject a <source> element for every srcset.<breakpoint alias> in the HTML markup of an <img> element contained in a <picture> elemen * support usages without `<picture>` parents * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * repackage API classes to easily distinguish flexbox APIs and extended responsive APIs Closes #366, Fixes #81, Fixes #376.

* add ImgSrcsetDirective * to inject <source> elements to support responsive images * Inject a <source> element for every srcset.<breakpoint alias> in the HTML markup of an <img> element contained in a <picture> elemen * support usages without `<picture>` parents * add responsive API to img.src: src.md, src.lt-lg, src.gt-xs, etc. * skip actions if responsive keys are not defined * without responsive keys (`src.<alias>`) defined, the ImgSrcDirective should *fall-through* and not change any attributes or properties on the `img` DOM element. The `img.src` attribute is dynamically set only when responsive keys are defined. * defaults to `src=""` if not explicitly assigned * responsive key activation will then assign the activated value to `img.src` attribute. Closes #366, Fixes #81, Fixes #376.

* add responsive API to img[src.md], img[src.lt-lg], img[src.gt-xs], etc. * skip actions if responsive keys are not defined * without responsive keys (`src.<alias>`) defined, the ImgSrcDirective should **fall-through** and not change any attributes or properties on the `img` DOM element. The `img.src` attribute is dynamically set only when responsive keys are defined. * defaults to `src=""` if not explicitly assigned * responsive key activation will then assign the activated value to `img.src` attribute. Closes #366, Fixes #81, Fixes #376.

angular-automatic-lock-bot · 2019-09-05T22:04:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Aug 6, 2017

ThomasBurleson self-requested a review August 6, 2017 13:49

ThomasBurleson added this to the v2.0.0-beta.9 milestone Aug 6, 2017

ThomasBurleson reviewed Aug 7, 2017

View reviewed changes

benbraou force-pushed the benbraou/fix-issue-81 branch from d65e00a to 0680d15 Compare August 8, 2017 10:29

ThomasBurleson suggested changes Aug 8, 2017

View reviewed changes

benbraou force-pushed the benbraou/fix-issue-81 branch from 0680d15 to 9a7c5c3 Compare August 8, 2017 19:06

benbraou force-pushed the benbraou/fix-issue-81 branch from 9a7c5c3 to 9498e2c Compare August 9, 2017 18:04

ThomasBurleson approved these changes Aug 9, 2017

View reviewed changes

ThomasBurleson added pr: lgtm This PR has been approved by the reviewer pr: needs presubmit labels Aug 9, 2017

ThomasBurleson suggested changes Aug 9, 2017

View reviewed changes

ThomasBurleson added in-progress and removed pr: lgtm This PR has been approved by the reviewer pr: needs presubmit labels Aug 9, 2017

benbraou force-pushed the benbraou/fix-issue-81 branch from 9498e2c to ebc9eb6 Compare August 10, 2017 04:54

ThomasBurleson approved these changes Aug 10, 2017

View reviewed changes

ThomasBurleson added the pr: lgtm This PR has been approved by the reviewer label Aug 10, 2017

ThomasBurleson mentioned this pull request Aug 17, 2017

feat(api): add responsive API for img elements #382

Merged

ThomasBurleson modified the milestones: v2.0.0-beta.10, v2.0.0-beta.9 Aug 17, 2017

ThomasBurleson added in-progress and removed pr: lgtm This PR has been approved by the reviewer labels Aug 17, 2017

andrewseguin closed this in #382 Aug 18, 2017

ThomasBurleson mentioned this pull request Aug 18, 2017

feat(img-srcset): add ImgSrcsetDirective for progressive images #383

Closed

ThomasBurleson mentioned this pull request Aug 24, 2017

feat(api): add responsive API for img elements #384

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(srcset): add srcset directive to inject <source> elements to sup… #366

feat(srcset): add srcset directive to inject <source> elements to sup… #366

benbraou commented Aug 6, 2017 •

edited

Loading

benbraou commented Aug 6, 2017

ThomasBurleson Aug 6, 2017

ThomasBurleson Aug 8, 2017

mackelito Aug 8, 2017

benbraou Aug 8, 2017

ThomasBurleson Aug 8, 2017

benbraou Aug 8, 2017

ThomasBurleson Aug 8, 2017

benbraou Aug 8, 2017

ThomasBurleson Aug 8, 2017

benbraou Aug 8, 2017

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

benbraou commented Aug 8, 2017 •

edited

Loading

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

benbraou commented Aug 9, 2017

ThomasBurleson commented Aug 9, 2017 •

edited

Loading

benbraou commented Aug 9, 2017

ThomasBurleson commented Aug 9, 2017

ThomasBurleson Aug 9, 2017

benbraou Aug 10, 2017

benbraou Aug 10, 2017

ThomasBurleson Aug 10, 2017

ThomasBurleson commented Aug 10, 2017

benbraou commented Aug 10, 2017

benbraou commented Aug 15, 2017

ThomasBurleson commented Aug 15, 2017

benbraou commented Aug 15, 2017 •

edited

Loading

ThomasBurleson commented Aug 15, 2017

ThomasBurleson commented Aug 17, 2017

ThomasBurleson commented Aug 17, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

feat(srcset): add srcset directive to inject <source> elements to sup… #366

feat(srcset): add srcset directive to inject <source> elements to sup… #366

Conversation

benbraou commented Aug 6, 2017 • edited Loading

benbraou commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Aug 8, 2017 • edited Loading

benbraou commented Aug 8, 2017 • edited Loading

ThomasBurleson commented Aug 8, 2017 • edited Loading

img-srcset.ts

ThomasBurleson commented Aug 8, 2017 • edited Loading

benbraou commented Aug 9, 2017

ThomasBurleson commented Aug 9, 2017 • edited Loading

benbraou commented Aug 9, 2017

ThomasBurleson commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasBurleson commented Aug 10, 2017

benbraou commented Aug 10, 2017

benbraou commented Aug 15, 2017

ThomasBurleson commented Aug 15, 2017

benbraou commented Aug 15, 2017 • edited Loading

ThomasBurleson commented Aug 15, 2017

ThomasBurleson commented Aug 17, 2017

ThomasBurleson commented Aug 17, 2017

angular-automatic-lock-bot bot commented Sep 5, 2019

benbraou commented Aug 6, 2017 •

edited

Loading

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

benbraou commented Aug 8, 2017 •

edited

Loading

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

ThomasBurleson commented Aug 8, 2017 •

edited

Loading

ThomasBurleson commented Aug 9, 2017 •

edited

Loading

benbraou commented Aug 15, 2017 •

edited

Loading