Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Automatically call responsive-loader, generate srcset from sizes #106

Closed
wants to merge 1 commit into from

Conversation

callumacrae
Copy link

@callumacrae callumacrae commented Dec 13, 2016

Regarding: #73

Syntax being called doesn't actually exist yet: dazuaz/responsive-loader#26 it's in a PR: dazuaz/responsive-loader#31

This is in theory a breaking change, but only if people are using sizes without srcset already (this makes no sense). Also, we should talk about whether this should be a plugin / another loader :)

@jsf-clabot
Copy link

jsf-clabot commented Dec 13, 2016

CLA assistant check
All committers have signed the CLA.

@callumacrae
Copy link
Author

Oooops, left some ES6 in sizes-extent by mistake. will fix that 😄

@codecov-io
Copy link

codecov-io commented Dec 14, 2016

Codecov Report

Merging #106 into master will increase coverage by 0.62%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   96.93%   97.56%   +0.62%     
==========================================
  Files           2        2              
  Lines          98      123      +25     
  Branches       19       23       +4     
==========================================
+ Hits           95      120      +25     
  Misses          3        3
Impacted Files Coverage Δ
index.js 96.96% <100%> (+0.37%) ⬆️
lib/attributesParser.js 100% <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 dac21c2...7befd76. Read the comment docs.

@hemanth
Copy link
Contributor

hemanth commented Feb 7, 2017

Any updates on this?

@callumacrae
Copy link
Author

Been super busy with a couple other things, but am definitely planning on completing this at some point.

@callumacrae
Copy link
Author

Working on this today 😄

Copy link
Member

@jhnns jhnns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature would be awesome 😁 👍

index.js Outdated
var hrefWithoutParams = href.slice(0, href.indexOf('?'));

var requireSizes = JSON.stringify(
'responsive' + params + '!' + loaderUtils.urlToRequest(hrefWithoutParams, root)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be responsive-loader, because webpack 2 doesn't append -loader automatically

index.js Outdated
throw new Error('sizes attribute not understood: ' + data[match].sizes);
}

var responsive = 'responsive?min=' + extent[0] + '&max=' + (extent[1] * MAX_DPR) + '!';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

@callumacrae
Copy link
Author

@jhnns: Will make that change.

Currently waiting on dazuaz/responsive-loader#31

@callumacrae
Copy link
Author

dazuaz/responsive-loader#31 has been merged which means I can now finish this PR 🎉 will do it at some point this week

@callumacrae
Copy link
Author

I've rebased the PR to get rid of conflicts. I have no idea why the tests are failing though - something to do with js-beautify.

@felipenmoura
Copy link

How is the current state for it?

It would be perfect it the html-loader could "replace" <img src="some-image.jpg" /> by the srcSet :)

@callumacrae
Copy link
Author

This PR was taking a lot of my time and it turned out I was actually working on the wrong project: html-loader isn't used for Vue templates. I think it would probably be best if someone else took this PR over, as I can't justify dedicating spending any more time at work on this when we're not actually going to benefit from it!

@michael-ciniawsky michael-ciniawsky removed this from the 0.6.0 milestone Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants