-
Notifications
You must be signed in to change notification settings - Fork 768
feat(img-srcset): add ImgSrcsetDirective for progressive images #383
Conversation
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.
@benbraou - Due to changes in master, it was easier to create a new PR that you and I can work on together. Thanks. |
@ThomasBurleson No problem. This has been a great learning experience. |
fixture.detectChanges(); | ||
expect(img).toBeDefined(); | ||
expect(img).toHaveAttributes({ | ||
src: fixture.componentInstance.mdSrcSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasBurleson With this test, we check that the src attribute is updated. However, srcset is still in the dom (with the old value corresponding to xsSrcSet). In case srcset value uses the pixel density 1x, it will have the priority, and we will not have the intended behaviour. I think that the test should be updated to do validation on both src and srcset. But most importantly, what should we do? Update in this case the srcset value and not introduce src? Or update both srcset and src?
|
||
// Only responsively update srcset values for stand-alone image elements | ||
// Fallback to [srcset] value for responsive deactivation | ||
this._listenForMediaQueryChanges('src', '', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key should be 'srcset' rather than 'src' otherwise onMediaQueryChange will never be triggered since inputKeyMap contains only srcset based keys
matchMedia.activate('xs'); | ||
fixture.detectChanges(); | ||
expect(defaultSourceEl).toHaveAttributes({ | ||
srcset: 'https://dummyimage.com/400x200/c7c224/000.png&text=default' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the source element have the srcset value 'https://dummyimage.com/500x200/76c720/fff.png&text=md' (and the media query corresponding to md) ?
When 'xs' is activated, the injected source element should not be updated. The browser will default to the img src value.
@benbraou - I am OoO next week. Meanwhile let us first document how this responsive |
Sure. My proposed wiki contribution will be in https://github.com/benbraou/flex-layout-wiki |
Closing. Will resubmit with improved logic and revised approach. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Support responsive image by introducing srcset. directive. When used as
<img>
attributes where<img>
is a child of a<picture>
container, this directive will injects a<source>
element for every srcset..<img>
is NOT nested in a<picture>
element, then theimg.src
property will be responsively updated.Closes #81.