Added min, max and steps arguments as an alternative to sizes #31
Conversation
|
@herrstucki: I'm not sure what's going on with the tests here - could you help me out? |
|
@callumacrae You're getting syntax errors in older versions of Node, See Travis logs. I ran into the same problem in a previous PR, see our discussion. I had to add a |
|
Got it. I was confused because I thought the syntax error was in the test file, not index.js. I've moved the use strict statement from the previous PR to cover the entire function, or it's not possible to use |
|
Anything I can do to get this merged? |
|
@alex-ketch @herrstucki |
|
@TomDeBacker I'm afraid you'll have to wait for @herrstucki to chime in. I know he's been working on a large PR #29, so he might want to merge that before this. Sorry for the wait, and thank you for the work @callumacrae. |
|
Yeah, I'm sorry for the delay! 😓 I've been pretty busy with other things. First impression is good, although I'm not yet 100% convinced that it's super-necessary to add more API surface for this relatively trivial use case. But if this is what a lot of people want, I won't say no I guess 😁 Three small things:
As @alex-ketch wrote, #29 is a bigger change but this doesn't depend on it, we just need to make sure we don't forget to add it there as well. |
|
Not rounding probably wouldn't be a problem (I'm honestly not sure), but I've told it to round up so that nothing unexpected happens. I've also changed the order of precedence and added an entry to the changelog. I never really understood how adding sizes to the webpack config file would have solved the problem: if I have an image which displays full width up to 1280px (*2 for retina), and I have an image which takes up half of an 1000px wide grid—500px—then setting the sizes in the webpack config will only work for one of them. |
|
Sorry to bump again @herrstucki, but is there anything else I can do to get this merged? |
|
@herrstucki would you like me to send a PR to the |
|
👋👋👋 @herrstucki, I'm Sean from the webpack team. If there's anything we can do to help support this PR, let us know. Nice work @callumacrae |
|
Hi @callumacrae, sorry again and thanks for your patience. I just merged and shipped the sharp branch as 👋 and 🙇 for the offer @TheLarkInn |
callumacrae
force-pushed
the
min-and-max
branch
2 times, most recently
from
dfb90ef
to
753f107
Compare
|
@herrstucki done! I started again from master instead of rebasing, but I guess it's had the same effect. I've never used jest or flow before, so let me know if I'm doing anything wrong there. |
|
Thanks @callumacrae, this looks good 👍 . You missed an edge case where setting |
|
@herrstucki allow edits from maintainers is already checked, but i've given you collaborator access to my repo now too. |
|
@callumacrae it seems the git origin wasn't set up properly. Sorry for the confusion 😄 |
|
Cool, those changes look good 👍 Any particular reason you're not willing to have an .editorconfig file in this project? or is it just not suitable for this PR? |
|
@callumacrae I'm generally not against having it but I assumed it slipped in by accident and wasn't really related to this PR. Also, I'd probably rather use prettier to format the code. Not sure if both are needed. Thanks again! Released as |
|
Awesome 🎉 thanks! |
closes #26