Skip to content

Reduce imageHeight to windowHeight if greater#7388

Merged
deetergp merged 2 commits into
Expensify:mainfrom
sobitneupane:sn_bug-attachment-height
Jan 26, 2022
Merged

Reduce imageHeight to windowHeight if greater#7388
deetergp merged 2 commits into
Expensify:mainfrom
sobitneupane:sn_bug-attachment-height

Conversation

@sobitneupane

@sobitneupane sobitneupane commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

@sobitneupane Please add screenshots for other platforms.

@sobitneupane

Copy link
Copy Markdown
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
Copy Markdown
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

sobitneupane commented Jan 25, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

@parasharrajat parasharrajat left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

OSBotify commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

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

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

@OSBotify

OSBotify commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

🚀 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