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

Fix filters image ratio display #254

Closed
wants to merge 16 commits into from
Closed

Fix filters image ratio display #254

wants to merge 16 commits into from

Conversation

skjnldsv
Copy link

@skjnldsv skjnldsv commented Aug 17, 2022

Before After
Peek 17-08-2022 18-04 Peek 17-08-2022 18-03

Fix #253
Fix nextcloud/viewer#1335

@skjnldsv
Copy link
Author

skjnldsv commented Sep 13, 2022

Ping @amrelbialy :)

EDIT, maybe @amrw-js ?

@amrelbialy
Copy link

Hey @skjnldsv, We don't consider this as a bug as it was implemented this way. The filters don't need to be aligned with image ratio as we are showing small fixed preview and when you click on any filter you can see the correct ration of the image with correct filter.

@skjnldsv
Copy link
Author

@amrelbialy squashing the images preview is not a bug to you?
This makes this UI looks broken. If you're showing a preview, make sure you're keeping its ratio, no?
I'm also fine if you prefer to crop the image into a square. But squashing and changing the image ratio is definitely not acceptable I think 😉

We don't consider this as a bug as it was implemented this way

We could say that to literally all bugs existing in the world 😁

@skjnldsv
Copy link
Author

skjnldsv commented Oct 18, 2022

Here is a more realistic example
image

image

Copy link

@nimishavijay nimishavijay left a comment

Choose a reason for hiding this comment

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

Agreed with @skjnldsv, cropping the image for a preview is okay, but warping it may give a wrong impression of what the filter would do :) a cropped square preview would be useful when there are multiple aspect ratios in a row, but since all the previews are of the same image, we don't have to crop it either. So nice work @skjnldsv :)

Copy link

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks much better :)

AhmeeedMostafa
AhmeeedMostafa previously approved these changes Nov 2, 2022
@AhmeeedMostafa AhmeeedMostafa changed the base branch from master to v4-dev November 2, 2022 11:49
@AhmeeedMostafa AhmeeedMostafa self-requested a review November 2, 2022 12:13
Copy link
Collaborator

@AhmeeedMostafa AhmeeedMostafa left a comment

Choose a reason for hiding this comment

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

Code is approved but after reviewing the PR's UI & Behavior it's better to replace with center cropping behavior for aligning UI dimensions in all cases considering small & large screens.

@AhmeeedMostafa AhmeeedMostafa dismissed their stale review November 2, 2022 13:01

Cropping behavior will be considered in the new release

@amrelbialy amrelbialy closed this Nov 2, 2022
@skjnldsv skjnldsv deleted the fix/filters-ratio branch November 2, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants