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 aspect ratio module #1454

Merged
merged 17 commits into from
Jan 16, 2020
Merged

Added aspect ratio module #1454

merged 17 commits into from
Jan 16, 2020

Conversation

root00198
Copy link
Member

This module helps to crop an image, on the basis of the resulting aspect ratio.

  • In Browser Usage

  • inbrowser usage

  • In Command Line Interface

  • cli usage

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
  • Insert-step functionality is working correct as expected.

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
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!

@codecov
Copy link

codecov bot commented Jan 11, 2020

Codecov Report

Merging #1454 into main will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1454   +/-   ##
=====================================
  Coverage   65.5%   65.5%           
=====================================
  Files        128     128           
  Lines       2618    2618           
  Branches     420     420           
=====================================
  Hits        1715    1715           
  Misses       903     903
Impacted Files Coverage Δ
src/modules/Average/Module.js 100% <100%> (ø) ⬆️

@root00198
Copy link
Member Author

@publiclab/is-reviewers

@keshav234156
Copy link
Member

keshav234156 commented Jan 11, 2020

@jywarren @harshkhandeparkar @root00198 Do we actually need this as a separate module if we have already resize module? Though implementing aspect ratio is a good idea but it would be better if we have within resize module and we already have an issue for it #1298

@root00198
Copy link
Member Author

root00198 commented Jan 11, 2020

@keshav234156 As far as I can understand, resize module reduces the width and height of the image according to a certain percentage, whereas the aspect ratio module changes the height and width of the image by croping it and aspect ratio module also consider the point from where to start the croping which resize module might not.

@rishabhshuklax
Copy link
Member

Yes I agree with @keshav234156 but we can have this as an addition in crop module, there we could provide an input field for aspect ratio, what do you think?

@root00198
Copy link
Member Author

@harshkhandeparkar @jywarren what do you say?

@harshkhandeparkar
Copy link
Member

Hey, how about making this a meta module? You won't have to write any extra code.
@root00198 ?

@root00198
Copy link
Member Author

I would have to write some code regardless, because of the calculations (regarding the resulting width and height) needed to be done before passing the execution to the crop module.

@harshkhandeparkar
Copy link
Member

Do meta modules use internal sequencer? If yes, then this would faster. You know what, don't use meta modules.

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

Do you think AspectRatioCrop or RatioCrop or LockedCrop would be a better name?

@root00198
Copy link
Member Author

AspectRatioCrop feels more like it. @harshkhandeparkar what do you say, should I change the module name?

@harshkhandeparkar
Copy link
Member

Sure, change it.

@harshkhandeparkar
Copy link
Member

Wait!

@harshkhandeparkar
Copy link
Member

I just checked. Shotwell photo viewer calls the normal crop, unconstrained. How about calling our new module ConstrainedCrop?

@root00198
Copy link
Member Author

yeah that feels much better. let's do it.

@harshkhandeparkar
Copy link
Member

👍

src/modules/ConstrainedCrop/Module.js Outdated Show resolved Hide resolved
src/modules/ConstrainedCrop/Module.js Outdated Show resolved Hide resolved
@root00198
Copy link
Member Author

Also working on adding the test for this module.

@harshkhandeparkar
Copy link
Member

Awesome!

@harshkhandeparkar
Copy link
Member

LGTM. Just waiting for travis to complete.

@jywarren jywarren merged commit 2736b48 into publiclab:main Jan 16, 2020
@jywarren
Copy link
Member

Great work here! Do we feel that this is similar enough to the regular crop that it could be included as a configurable param for that module?

@root00198
Copy link
Member Author

It could be included as a part of the crop module.

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.

5 participants