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

Reduce imageHeight to windowHeight if greater #7388

Merged
merged 2 commits into from
Jan 26, 2022

Conversation

sobitneupane
Copy link
Contributor

@sobitneupane sobitneupane commented Jan 25, 2022

Details

Reduce height of image to height of window if it is greater than window's height.

Fixed Issues

$ #7012

Tests

  • Send a large image
  • Open attachment to see if image height fits within the window

QA Steps

  • Send a large image
  • Open attachment to see if image height fits within the window

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot from 2022-01-25 14-21-45

Mobile Web

Screenshot_20220125-142126_Chrome

Desktop

iOS

Android

Screenshot_20220125-145141_New Expensify

@sobitneupane sobitneupane requested a review from a team as a code owner January 25, 2022 09:48
@MelvinBot MelvinBot requested review from deetergp and parasharrajat and removed request for a team January 25, 2022 09:48
@parasharrajat
Copy link
Member

@sobitneupane Please add screenshots for other platforms.

@sobitneupane
Copy link
Contributor Author

@sobitneupane Please add screenshots for other platforms.

I have added for Web and mWeb. I don't have accessibility of macOS and IOS.

@parasharrajat
Copy link
Member

Ok, I tested the changes. But now when we zoom the image it does not take the full width of the screen.
image

@sobitneupane
Copy link
Contributor Author

sobitneupane commented Jan 25, 2022

Ok, I tested the changes. But now when we zoom the image it does not take the full width of the screen.

It is because we have used 2 as scale to zoomin for doubleClick. It can be zoomed further using pinch-to-zoom.

if (isDoubleClick) {
    this.zoom.centerOn({
        x: 0,
        y: 0,
        scale: 2,
        duration: 100,
    });
}

@deetergp
Copy link
Contributor

Overall the PR looks good and I greatly appreciate the comments explaining the "why" of each bit of logic. Thanks! You've got some merge conflicts you need to resolve, but once that's done it will be good to go, IMO!

parasharrajat
parasharrajat previously approved these changes Jan 25, 2022
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM:

cc: @deetergp

🎀 👀 🎀 C+ reviewed

@deetergp deetergp merged commit fb8a723 into Expensify:main Jan 26, 2022
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Feb 1, 2022

🚀 Deployed to staging by @deetergp in version: 1.1.33-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Feb 2, 2022

🚀 Deployed to production by @sketchydroide in version: 1.1.34-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants