Skip to content

feature: #176, insert_picture: added optional parameters crop & vcenter#439

Open
aatwork wants to merge 3 commits intoscanny:masterfrom
aatwork:feature/image_fitting
Open

feature: #176, insert_picture: added optional parameters crop & vcenter#439
aatwork wants to merge 3 commits intoscanny:masterfrom
aatwork:feature/image_fitting

Conversation

@aatwork
Copy link

@aatwork aatwork commented Sep 20, 2018

This change enables the functionality requested in #176

The change is backward compatible because the defaults for the new arguments correspond to the previous behavior.

if crop argument is set to False, the image will be scaled proportionally to be able to fit into the placeholder

if vcenter is set to True, the image will be centered vertically

@adamerose
Copy link

@aatwork Could you also add an hcenter flag to this? You did most of the work already it just requires the same logic and arguments as vcenter, like the following at line 349

if aspectPh > aspectImg:
    w = int(ph_h * aspectImg)
    h = ph_h  # keep the height

    if hcenter:
        left = left + (ph_w - w) / 2

And add hcenter to the arguments at lines 313, 323, 327

@loscil06
Copy link

Incredibly useful, thanks a lot.

@aatwork
Copy link
Author

aatwork commented Dec 23, 2018

@adamerose
I have added the hcenter flag as requested

@mikebirdgeneau
Copy link

mikebirdgeneau commented Mar 20, 2019

It looks like the tests need to be updated to account for the new function arguments. I don't have any experience with this type of thing, but it looks like something such as this:
tests/shapes/test_placeholder.py @ line 508:

picture_ph._new_placeholder_pic.assert_called_once_with(image_file, True, False, False)

Might fix the tests so the travis-ci build succeeds. Perhaps worth updating your pull request with this or something better that achieves the same result.

@sgorchichko
Copy link

@scanny , Any chances this will be merged?

@pbregener
Copy link

This feature would be great to have!

This PR needs a small test fix and a rebase to get rid of the conflict.

Assuming @aatwork (or someone else) takes care of these minor issues, would you consider merging this @scanny ?

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.

6 participants