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

fix handling of 'image' parameter #54

Closed
wants to merge 5 commits into from
Closed

fix handling of 'image' parameter #54

wants to merge 5 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 20, 2017

seems to be a trivial bug.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 89.051% when pulling f8795c7 on yamaryoxxxx:master into 2810732 on jpmonette:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 88.971% when pulling 00d3882 on yamaryoxxxx:master into 2810732 on jpmonette:master.

@jpmonette
Copy link
Owner

@yamaryoxxxx Thanks for your time.

This can cause an issue with .jpg file format (as an example) since the correct mime-type is image/jpeg and not image/jpg. I believe using a library such as https://www.npmjs.com/package/mime-types would be a bit more solid.

I would be open to merge if you provide as a source the need supporting that change + if it uses the library above or another solid library.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.05%) to 89.286% when pulling 27f4eaa on yamaryoxxxx:master into 2810732 on jpmonette:master.

@ghost
Copy link
Author

ghost commented May 20, 2017

I would be open to merge if you provide as a source the need supporting that change

I think #37 seems to be an example.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.05%) to 89.286% when pulling cd9d0e4 on yamaryoxxxx:master into 2810732 on jpmonette:master.

@jpmonette
Copy link
Owner

@yamaryoxxxx I'm curious about what has been changed in the package.json? I tried to revert the changes since this file is outside the scope of that change. Can you exclude it from the PR or create another PR with only the test file, updated feed.js and the compiled JS files?

Thanks.

@jpmonette
Copy link
Owner

Considering the impact on multiple unrelated files and no fix, will close this PR.

@jpmonette jpmonette closed this Nov 18, 2017
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.

3 participants