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

Added new method to resize images #63

Closed
wants to merge 4 commits into from
Closed

Added new method to resize images #63

wants to merge 4 commits into from

Conversation

Hoppi164
Copy link

Hi there

There is an issue when trying to resize images using gulp
I encountered the same error that is listed in this issue #37

This seems to be an internal issue with gulp which completely stops the 'resizing' functionality from working.
I've designed an alternate method to efficiently resize the images into "full" and "thumb" folders.
you can execute this script by running npm run resize

package.json Outdated Show resolved Hide resolved
@rampatra
Copy link
Owner

Thanks a mil for raising this PR and wanting to improve this project. It looks good to me and can merge it once we have some more 👍 from other users. I hope that's okay?

@Hoppi164
Copy link
Author

My pleasure!, and yeah that's totally fine by me :)

@Hoppi164
Copy link
Author

Hi Ram.

It's been 5 months, and I wanted to check-in on this.

I'm still okay for this to be merged if you're happy with it?

If you've decided not to progress with this approach: please let me know and I'll close out this PR

Kind Regards
Drew

@Hoppi164 Hoppi164 closed this Mar 23, 2022
@rampatra
Copy link
Owner

Hi @Hoppi164 ,

Apologies for not responding earlier. I can merge it now if you're still interested? Also, did you try setting it as a devDependency instead?

@rampatra rampatra reopened this Mar 23, 2022
@Hoppi164
Copy link
Author

Hey @rampatra

All good mate, No need to apologize,
I closed this PR because i felt it had gotten stale, and i wanted to clean up my list of PR's
I'm still happy for you to merge this PR.

I changed the "sharp" dependancy to a "dev dependancy" in this second commit d263ede

Kind Regards
Andrew

@rampatra
Copy link
Owner

rampatra commented Mar 23, 2022

That's great @Hoppi164 so can we please get rid of the unused dependency block in package.json ?

Update: I modified it.

resizeImages.js Outdated Show resolved Hide resolved
// Helper function to resize images using sharp
async function resize(fileName, size) {
let sizeMap = {
'full' : {width: 1024, height:768, location:'./images/fulls'},
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to specify a height here and just the width? This will maintain the aspect ratio of the images I think?

Copy link
Author

Choose a reason for hiding this comment

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

Hi there,

Do you mean specifying JUST the height?
Like below?

let sizeMap = {
    'full' : { height:768, location:'./images/fulls'},
    'thumb' : { height:256, location:'./images/thumbs'}
}

Copy link
Author

Choose a reason for hiding this comment

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

I feel it might be better to maintain the aspect ratio based on the width instead.
I think that's what you're doing in the previous gulpfile too.

Are you happy with the below width approach?

let sizeMap = {
    'full' : { width:1024, location:'./images/fulls'},
    'thumb' : { width:512, location:'./images/thumbs'}
}

@Hoppi164
Copy link
Author

Hi Ram,

Oh i see what you mean with the package.json empty dependancy
Sorry for the slow reply, and thanks for fixing that.

@Hoppi164 Hoppi164 closed this Jun 19, 2022
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.

2 participants