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

Support for srcset #953

Merged
merged 4 commits into from Oct 11, 2017
Merged

Support for srcset #953

merged 4 commits into from Oct 11, 2017

Conversation

AndreKR
Copy link

@AndreKR AndreKR commented Aug 23, 2017

This PR adds support for <img srcset="...">. The srcset URLs are converted to require()s, just as <img src="..."> is, the descriptors stay untouched.

@narrowtux
Copy link

Would love to have this pulled, when can I expect this?

@AndreKR
Copy link
Author

AndreKR commented Sep 6, 2017

As a workaround I'm currently putting

"vue-loader": "AndreKR/vue-loader#srcset",

in my package.json.

Note however, that I'm not actively monitoring vuejs/vue-loader to pull in updates.

@therealpecus
Copy link

Hi @AndreKR,
thank you for this. We tested it and discovered a couple of issues: your code does not take into account attributes spawning multiple lines, and srcset attributes on source tags (useful for picture elements).

I made a quick review of your code that I submitted as a PR, but I think credit fir this should be yours, so feel free to review them and update this PR.

For all of those that can't wait, see https://github.com/therealpecus/vue-loader srcset branch.

covers cases like:
```
<img srcset="
    ../image.jpg 1x, 
    ../image.2x.jpg 2x,
    ../image.1920x1280.jpg 1920w
" alt="srcset on multiple lines and with extra whitespace">
```
Also added a test for srcset.
@AndreKR
Copy link
Author

AndreKR commented Sep 19, 2017

@therealpecus Thanks for the fixes, I cherry-picked your commit into this branch. I found another issue with whitespace within the candidates. I also added a test for all three cases. Please check again.

@therealpecus
Copy link

Hopefully this will speed up the approval process. Thanks for the further review!

@assembledadam
Copy link

Any word on merging this? Thanks.

@narrowtux
Copy link

bump

@yyx990803 yyx990803 merged commit 69fd7b0 into vuejs:master Oct 11, 2017
@assembledadam
Copy link

assembledadam commented May 7, 2018

Whilst this was merged, it doesn't appear to be in any stable version of vuejs loader... I'm still having to use AndreKR's forked version. Any plans?

@gburning
Copy link

Any word on how to get this working?

@hmaesta
Copy link

hmaesta commented Oct 23, 2018

14 months and counting. 😥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants