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

Add minify-image module #1149

Merged
merged 7 commits into from Jul 9, 2019
Merged

Conversation

Divy123
Copy link
Member

@Divy123 Divy123 commented Jul 3, 2019

Fixes #1142

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@Divy123
Copy link
Member Author

Divy123 commented Jul 3, 2019

@jywarren please have a look, rebased changes here.
Thanks!!

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #1149 into main will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
+ Coverage   55.39%   55.66%   +0.27%     
==========================================
  Files         113      114       +1     
  Lines        2356     2357       +1     
  Branches      364      364              
==========================================
+ Hits         1305     1312       +7     
+ Misses       1051     1045       -6
Impacted Files Coverage Δ
src/Modules.js 100% <ø> (ø) ⬆️
src/modules/MinifyImage/index.js 100% <100%> (ø)
src/ui/UserInterface.js 70.83% <0%> (+25%) ⬆️

@Divy123
Copy link
Member Author

Divy123 commented Jul 3, 2019

Screenshot from 2019-07-03 17-29-00
Here is the error and also one of the modules is using fs.
Can you please help me on this?
Thanks !!

@Divy123
Copy link
Member Author

Divy123 commented Jul 3, 2019

@jywarren please have a look.

@Divy123
Copy link
Member Author

Divy123 commented Jul 4, 2019

@jywarren please review this. Ready for final review.

}

// write the ArrayBuffer to a blob, and you're done
var blob = new Blob([ab], {
Copy link
Member

Choose a reason for hiding this comment

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

This could be written just like return new Blob([ia], { type: mimeString }); if you want.

Copy link
Member Author

@Divy123 Divy123 Jul 9, 2019

Choose a reason for hiding this comment

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

Thanks @IgorWilbert
Done that for better readability.

function dataURItoBlob(dataURI) {
// convert base64 to raw binary data held in a string
// doesn't handle URLEncoded DataURIs - see SO answer #6850276 for code that does this
var byteString = atob(dataURI.split(',')[1]);
Copy link
Member

Choose a reason for hiding this comment

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

No need for this?

var byteString;
if (dataURI.split(',')[0].indexOf('base64') >= 0)
      byteString = atob(dataURI.split(',')[1]);
    else
byteString = unescape(dataURI.split(',')[1]);

Copy link
Member Author

Choose a reason for hiding this comment

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

as that completely works!!

@IgorWilbert
Copy link
Member

Awesome job @Divy123 ! Just left some comments that are more like doubts, I hope they happen to be useful. Thank you!

@jywarren jywarren mentioned this pull request Jul 9, 2019
4 tasks
@Divy123
Copy link
Member Author

Divy123 commented Jul 9, 2019

@jywarren please review!!

@jywarren
Copy link
Member

jywarren commented Jul 9, 2019

Great, good to merge once conflicts are resolved, thanks!

@jywarren jywarren merged commit 1b26f71 into publiclab:main Jul 9, 2019
@jywarren
Copy link
Member

jywarren commented Jul 9, 2019

🎉 !!!!! Great work!

jywarren pushed a commit that referenced this pull request Dec 16, 2019
* Add minify-image module

* Add node modules

* corrected path

* Add browser func

* Add test and documentation
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.

Minify Image module
3 participants