feat: add `srcset` support (`<img>`) #147

ushu · 2017-10-03T14:09:03Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

srcset attribute is not loaded from the HTML content

What is the new behavior?

srcset attribute (actually the images inside the srcset declaration) is loaded

Does this PR introduce a breaking change?

Yes
No

Other information:

I did this code before realising that an old PR (#55) has been addressing the same issue.
So the code ended up being quite different.

The added value of this version is that it supports multiple qualifiers for the same source such as "100w 2x", which are useful when coding for the mobile web.

Also I am not why this feature has been removed but since I have some time his week I can iterate on the PR with some guidance.

Regards !

jsf-clabot · 2017-10-03T14:09:04Z

All committers have signed the CLA.

codecov · 2017-10-03T14:10:52Z

Codecov Report

Merging #147 into master will increase coverage by 0.32%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   96.96%   97.29%   +0.32%     
==========================================
  Files           2        2              
  Lines          99      111      +12     
  Branches       20       22       +2     
==========================================
+ Hits           96      108      +12     
  Misses          3        3

Impacted Files	Coverage Δ
lib/attributesParser.js	`100% <100%> (ø)`	⬆️
index.js	`96.62% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b13d4c...4371750. Read the comment docs.

cupojoe · 2017-10-04T21:37:35Z

What is the status of this making it to release? @michael-ciniawsky

michael-ciniawsky

What happens if one of these asset urls doesn't exist ?
This maybe better implemented as an opt-in ?

michael-ciniawsky · 2017-10-04T23:31:02Z

lib/attributesParser.js

@@ -4,13 +4,36 @@
 */
 var Parser = require("fastparse");

+var srcsetQualifierRegexp = /(\s+(\d+w))?(\s+(\d+x))?\s*$/;


var SRCSET_REGEXP

michael-ciniawsky · 2017-10-04T23:32:15Z

lib/attributesParser.js

+
+	// allow to load several urls in srcsets, separated by commas
+	var start = index + strUntilValue.length;
+	var subMatches = value.split(/,/);


michael-ciniawsky · 2017-10-04T23:34:54Z

lib/attributesParser.js

+		var length = subMatch.length;
+
+		// remove initial spacing
+		var initialSpace = /^\s+/.exec(subMatch);


- var initialSpace = /^\s+/.exec(subMatch); - var spaceLength = initialSpace ? initialSpace[0].length : 0;

var space = { initial: /^\s+/.exec(subMatch), length: this.initial ? this.initial[0].length : 0 }

michael-ciniawsky · 2017-10-04T23:35:19Z

lib/attributesParser.js

+		var spaceLength = initialSpace ? initialSpace[0].length : 0;
+
+		// remove srcset qualifiers (2x, 110w, etc.), if any
+		var qualifier = srcsetQualifierRegexp.exec(subMatch);


michael-ciniawsky · 2017-10-04T23:37:25Z

lib/attributesParser.js

@@ -4,13 +4,36 @@
 */


Please move this into its own folder && file => ./lib/attrs/srcset.js and require('./attrs/srcset.js') it

michael-ciniawsky · 2017-10-04T23:42:33Z

@cupojoe It's in review and needs approval from 2+ maintainers, so no release date estimate can be made yet

joshwiens · 2017-10-05T01:01:49Z

For reference, one of those should be @hemanth - This is one of the loaders that actually has an assigned lead maintainer

hemanth

@michael-ciniawsky has covered most of it, apart of that LGTM.

joshwiens

Agreed, should be g2g with the changes @michael-ciniawsky has already suggested.

codecov-io · 2017-10-10T06:55:36Z

Codecov Report

Merging #147 into master will increase coverage by 0.57%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
+ Coverage   96.96%   97.54%   +0.57%     
==========================================
  Files           2        3       +1     
  Lines          99      122      +23     
  Branches       20       22       +2     
==========================================
+ Hits           96      119      +23     
  Misses          3        3

Impacted Files	Coverage Δ
lib/attrs/srcset.js	`100% <100%> (ø)`
lib/attributesParser.js	`100% <100%> (ø)`	⬆️
index.js	`96.62% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b13d4c...4371750. Read the comment docs.

ushu · 2017-10-10T07:04:18Z

Hey there,

I moved the code into attrs/srcset.js as requested, and also updated naming and general structure following your guidance to make the code more readable.

@michael-ciniawsky about:

What happens if one of these asset urls doesn't exist ?
This maybe better implemented as an opt-in ?

I'm not really sure... the idea in the code was really to load images from the srcset as if they were included in a src attribute and so I don't think it should accept mentions to non-existing resources.

I know of other attempts to auto-generate these responsive assets on-the-fly, but in my case I work with designers that produce these images (sometimes slightly modified for smaller res) and it is helpful to have webpack check if they are present, as it does for src.

just my two cents though,
Kind Regards

cupojoe · 2017-10-13T16:14:42Z

@michael-ciniawsky @ushu
Totally agree with @ushu. Assuming the build can generate the images is not a good idea. It should respect the existing sources to be more friendly towards integration with design.

michael-ciniawsky · 2018-01-04T20:30:58Z

Sry but I have to declined this since v1.0.0 of the loader will be a complete overhaul. You can use posthtml-loader && write your own/a plugin to achieve this

Since webpack-contrib/html-loader#147 was rejected, I changed the package name to allow to import the code in new projects easily.

ushu added 2 commits October 3, 2017 15:44

Load srcset by default

2879105

Load srcset syntax

ed05aa5

ushu added 4 commits October 3, 2017 16:19

Add support for multiple qualifiers: 100w 2x etc.

27f6048

Add test

6b467d2

Add doc, mostly copied from #55

18e090b

Normalize spaces in generated require statement

5fb5372

michael-ciniawsky changed the title ~~(re)Add Srcset support~~ feat: add srcset support (<img>) Oct 3, 2017

michael-ciniawsky added priority: 4 (important) pr_action: Review scope: Parser semver: Minor severity: 4 (inconvenient) status: Pending type: Feature labels Oct 3, 2017

michael-ciniawsky added this to the 0.6.0 milestone Oct 3, 2017

michael-ciniawsky requested changes Oct 4, 2017

View reviewed changes

michael-ciniawsky requested review from alexander-akait and joshwiens October 4, 2017 23:42

joshwiens requested a review from hemanth October 5, 2017 01:00

hemanth reviewed Oct 9, 2017

View reviewed changes

joshwiens reviewed Oct 9, 2017

View reviewed changes

Extract srcset logic into attrs/srcset.js

4371750

michael-ciniawsky closed this Jan 4, 2018

michael-ciniawsky added status: Declined and removed priority: 4 (important) pr_action: Review scope: Parser severity: 4 (inconvenient) status: Pending labels Jan 4, 2018

michael-ciniawsky removed this from the 0.6.0 milestone Jan 4, 2018

ushu added a commit to ushu/html-srcsets-loader that referenced this pull request Jan 30, 2018

Add support for srcset attribute(s)

2130d99

Since webpack-contrib/html-loader#147 was rejected, I changed the package name to allow to import the code in new projects easily.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `srcset` support (`<img>`) #147

feat: add `srcset` support (`<img>`) #147

ushu commented Oct 3, 2017

jsf-clabot commented Oct 3, 2017 •

edited

Loading

codecov bot commented Oct 3, 2017 •

edited

Loading

cupojoe commented Oct 4, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

michael-ciniawsky Oct 4, 2017

michael-ciniawsky Oct 4, 2017

michael-ciniawsky Oct 4, 2017

michael-ciniawsky Oct 4, 2017

michael-ciniawsky Oct 4, 2017

michael-ciniawsky commented Oct 4, 2017

joshwiens commented Oct 5, 2017

hemanth left a comment

joshwiens left a comment

codecov-io commented Oct 10, 2017 •

edited by codecov bot

Loading

ushu commented Oct 10, 2017 •

edited

Loading

cupojoe commented Oct 13, 2017

michael-ciniawsky commented Jan 4, 2018

feat: add srcset support (<img>) #147

feat: add srcset support (<img>) #147

Conversation

ushu commented Oct 3, 2017

jsf-clabot commented Oct 3, 2017 • edited Loading

codecov bot commented Oct 3, 2017 • edited Loading

Codecov Report

cupojoe commented Oct 4, 2017 • edited Loading

michael-ciniawsky left a comment • edited Loading

Choose a reason for hiding this comment

michael-ciniawsky Oct 4, 2017

Choose a reason for hiding this comment

michael-ciniawsky Oct 4, 2017

Choose a reason for hiding this comment

michael-ciniawsky Oct 4, 2017

Choose a reason for hiding this comment

michael-ciniawsky Oct 4, 2017

Choose a reason for hiding this comment

michael-ciniawsky Oct 4, 2017

Choose a reason for hiding this comment

michael-ciniawsky commented Oct 4, 2017

joshwiens commented Oct 5, 2017

hemanth left a comment

Choose a reason for hiding this comment

joshwiens left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 10, 2017 • edited by codecov bot Loading

Codecov Report

ushu commented Oct 10, 2017 • edited Loading

cupojoe commented Oct 13, 2017

michael-ciniawsky commented Jan 4, 2018

feat: add `srcset` support (`<img>`) #147

feat: add `srcset` support (`<img>`) #147

jsf-clabot commented Oct 3, 2017 •

edited

Loading

codecov bot commented Oct 3, 2017 •

edited

Loading

cupojoe commented Oct 4, 2017 •

edited

Loading

michael-ciniawsky left a comment •

edited

Loading

codecov-io commented Oct 10, 2017 •

edited by codecov bot

Loading

ushu commented Oct 10, 2017 •

edited

Loading