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

Images in srcset should be included too #7

Open
marijnvdwerf opened this issue Aug 10, 2015 · 3 comments
Open

Images in srcset should be included too #7

marijnvdwerf opened this issue Aug 10, 2015 · 3 comments

Comments

@marijnvdwerf
Copy link

Expected:
Image with both src and srcset has both images included

<img src="/images/logo.png" srcset="/images/logo@2x.png 2x" />

Actual:
Image only has src included as data URI. srcset is left unchanged

@tommedema
Copy link
Contributor

@jrit in addition to the above, favicons and fonts are also not embedded; do you plan to do this or welcome a PR for this? should there be config to enable/disable types of assets?

@jrit
Copy link
Owner

jrit commented Oct 1, 2017

@tommedema I'd welcome a PR, but don't plan to add that myself. My preference would be a config similar to how the existing configs work per asset type, and with a similar rationale for defaults.

@tommedema
Copy link
Contributor

@jrit if I ever get to this in the future, do you think that the current logic should be refactored to have a single detection and replacement engine in which differences for each type can be injected?

Currently the code is not very DRY, which is already apparent from a glance at the code:

screen shot 2017-12-23 at 13 18 55

This could be rewritten to 1 block of logic and 4 configuration entries. Wdyt?

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

No branches or pull requests

3 participants