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

[Feature] Introducing image compression using imagemin #654

Merged
merged 9 commits into from
May 9, 2018
Merged

[Feature] Introducing image compression using imagemin #654

merged 9 commits into from
May 9, 2018

Conversation

ahmadalfy
Copy link
Contributor

@ahmadalfy ahmadalfy commented May 8, 2018

Motivation

Static assets should always be compressed to improve loading speed and enhance user experience. The static images in the user folder was 4.4MB. After using imagemin the total size of the image was reduced to nearly the half.

Also this PR include replacing the images in the example website with the optimized ones to improve the download speed for future users.

Have you read the Contributing Guidelines on pull requests?

Yes

Related Issues

#568

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 8, 2018
@ahmadalfy
Copy link
Contributor Author

For some reason imagemin-svgo wasn't added to package.json. Let me fix that

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Code looks good except for some nits. Would be better to give a flag to users in case they want to opt out of the compression for faster build times. @JoelMarcey Thoughts?

@@ -399,6 +404,24 @@ async function execute() {
}

fs.writeFileSync(mainCss, cssContent);
} else if (normalizedFile.match(/\.png$|.jpg$|.svg$|.gif$/)) {
console.log(normalizedFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opps!

} else if (normalizedFile.match(/\.png$|.jpg$|.svg$|.gif$/)) {
console.log(normalizedFile);
let parts = normalizedFile.split(sep + 'static' + sep);
let targetDirectory = join(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was following the same convention used on the next block, should I update that one as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 8, 2018

Deploy preview for docusaurus-preview ready!

Built with commit eb00611

https://deploy-preview-654--docusaurus-preview.netlify.com

@JoelMarcey
Copy link
Contributor

Re: a flag, is the image quality that diminished that the average person would notice on a website? If so, a flag makes more sense to me. If not, then a flag is probably still good, but maybe less of a mandatory thing.

@ahmadalfy
Copy link
Contributor Author

This is a lossless compression, image quality is not affected as it is only removing unnecessary meta data like EXIF info and others.

@ahmadalfy
Copy link
Contributor Author

OK does the flag name --no-compression sound good ?

@yangshun
Copy link
Contributor

yangshun commented May 8, 2018

Let's call it --skip-image-compression. We'll have to document it too 👍

@JoelMarcey
Copy link
Contributor

Actually -- a name like no-image-compression would be more descriptive, I think. Otherwise people might ask what we are compressing.

@JoelMarcey
Copy link
Contributor

--skip-image-compression is good too.

@yangshun
Copy link
Contributor

yangshun commented May 8, 2018

I agree. Because we might potentially add flags for the CSS compression (and other types of assets).

@ahmadalfy
Copy link
Contributor Author

Perfect, will do

@ahmadalfy
Copy link
Contributor Author

ahmadalfy commented May 8, 2018

Does this sound good:

This will generate a `build` folder inside the `website` directory containing the `.html`
files from all of your docs and other pages included in `pages`. 

**** The new part below ****
The build command will
also compress all the images you place in your assets directory unless you supply it with
the flag `--skip-image-compression`.

@yangshun
Copy link
Contributor

yangshun commented May 9, 2018

No, that flag should be documented on this page - https://docusaurus.io/docs/en/commands.html#docusaurus-build

docusaurus-build [--skip-image-opt] or something.

@ahmadalfy
Copy link
Contributor Author

Flag updated and added to the docs.

@yangshun
Copy link
Contributor

yangshun commented May 9, 2018

@ahmadalfy CI and the preview build is failing and I'm not sure why. Could you try to build locally and make sure your code is prettified?

@ahmadalfy
Copy link
Contributor Author

I ran prettier and introduced a change, build working though through

cd website
yarn build

Should work now :)

@yangshun yangshun merged commit ab6bab9 into facebook:master May 9, 2018
@yangshun
Copy link
Contributor

Awesome! Thank you for the PR! Such a huge win for us 😄

@ahmadalfy
Copy link
Contributor Author

Thanks for the welcoming spirit 💘

yangshun pushed a commit that referenced this pull request May 10, 2018
* migrating to react 16

* Add WarriorJS to Docusaurus users (#656)

* Separate users data from code

* Add WarriorJS to Docusaurus users

* [Feature] Introducing image compression using imagemin (#654)

* Introduce imagemin for compressing images

* Replace original images with the optimized ones

* Add imagemin-svgo to dependencies

* Remove console statement, replace let with const

* Replace let with const

* Add --skip-image-compression

* Run Prettier

* Fix header

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

Successfully merging this pull request may close these issues.

5 participants