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 chat lightbox shadow appearing when scrolling #17664

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

clauxx
Copy link
Member

@clauxx clauxx commented Oct 17, 2023

fixes #17555

Summary

Removed the "rogue" shadow effect, which happens when the message inside the lightbox component is fully expanded and the user scrolls the content further (bounce effect).

Additionally, I noticed a few differences with the designs so added a few changes to the gestures/styles, namely:

  1. Gestures felt a bit weird because of using scrollview together with the pan handler e.g. can't drag down overflowing text since the scrollview takes over the touches from the pan handler (which is needed to allow scrolling the text up)
  2. Background opacity was "re-starting" on text (that doesn't get to the top of the screen) if dragging it repeatedly when already fully expanded
  3. When scrolling off-screen, the "draggable" bar should be hiding behind the header together with the text
  4. Text can't be dragged lower that the initial position anymore (was causing issues with opacity, etc)
  5. Changed the top/bottom gradients according to designs
  6. Pressing text that covers the whole screen will move it below the header, meaning also that pressing it when it is already scrolled behind the header will move it to the top as well. This is helpful for really long text, so the user can get to the beginning by pressing on it while being consistent with the expanding behaviour.
  7. Fixed styles and removed gestures for one-lined text, according to designs

@OmarBasem since you worked on it originally, please have a look to make sure I didn't miss/break anything.

Testing notes

Platforms

  • Android
  • iOS

Areas that maybe impacted

Functional
  • 1-1 chats
  • public chats
  • group chats

Steps to test

  • Open any chat
  • Send a message containing image and about 9 lines if text
  • Open the sent image
  • Start scrolling the image description
  • Continue scrolling after the whole description is expanded
  • Pay attention at shadow effect which appears during scrolling

Before and after screenshots comparison

Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-10-19.at.02.04.18.mp4
Figma (if available) iOS (if available) Android (if available)
Please embed Image/Video here of the before and after. Please embed Image/Video here of the before and after.

status: ready

@clauxx clauxx self-assigned this Oct 17, 2023
@status-im-auto
Copy link
Member

status-im-auto commented Oct 17, 2023

Jenkins Builds

Click to see older builds (53)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 13b3dcd #1 2023-10-17 15:27:17 ~7 min ios 📱ipa 📲
✔️ 13b3dcd #1 2023-10-17 15:29:51 ~9 min android-e2e 🤖apk 📲
✔️ 13b3dcd #1 2023-10-17 15:30:08 ~9 min android 🤖apk 📲
✔️ 13b3dcd #1 2023-10-17 15:31:15 ~10 min tests 📄log
✔️ 694bb7e #2 2023-10-17 16:33:53 ~6 min ios 📱ipa 📲
✔️ 694bb7e #2 2023-10-17 16:34:30 ~7 min android-e2e 🤖apk 📲
✔️ 694bb7e #2 2023-10-17 16:34:44 ~7 min android 🤖apk 📲
✔️ 694bb7e #2 2023-10-17 16:38:15 ~10 min tests 📄log
✔️ cfa728e #4 2023-10-18 22:45:51 ~5 min android 🤖apk 📲
✔️ cfa728e #4 2023-10-18 22:49:38 ~9 min android-e2e 🤖apk 📲
✔️ cfa728e #4 2023-10-18 22:49:46 ~9 min tests 📄log
✔️ cfa728e #4 2023-10-18 22:53:11 ~12 min ios 📱ipa 📲
✔️ 8b16f3b #6 2023-10-19 20:14:54 ~8 min tests 📄log
✔️ 8b16f3b #6 2023-10-19 20:16:16 ~10 min ios 📱ipa 📲
8b16f3b #6 2023-10-19 20:16:31 ~10 min android 📄log
8b16f3b #6 2023-10-19 20:16:31 ~10 min android-e2e 📄log
ea8a4aa #7 2023-10-19 20:42:38 ~5 min tests 📄log
✔️ ea8a4aa #7 2023-10-19 20:44:01 ~6 min android-e2e 🤖apk 📲
✔️ ea8a4aa #7 2023-10-19 20:44:04 ~6 min android 🤖apk 📲
✔️ ea8a4aa #7 2023-10-19 20:44:39 ~7 min ios 📱ipa 📲
✔️ ea8a4aa #8 2023-10-19 20:52:01 ~8 min tests 📄log
50336c8 #10 2023-10-20 22:37:07 ~2 min tests 📄log
✔️ 50336c8 #9 2023-10-20 22:40:55 ~6 min android-e2e 🤖apk 📲
✔️ 50336c8 #9 2023-10-20 22:40:55 ~6 min android 🤖apk 📲
✔️ 50336c8 #9 2023-10-20 22:42:27 ~7 min ios 📱ipa 📲
604182d #11 2023-10-23 15:09:12 ~2 min tests 📄log
✔️ 604182d #10 2023-10-23 15:11:58 ~5 min ios 📱ipa 📲
✔️ 604182d #10 2023-10-23 15:13:00 ~6 min android-e2e 🤖apk 📲
✔️ 604182d #10 2023-10-23 15:13:03 ~6 min android 🤖apk 📲
✔️ df88fd2 #11 2023-10-23 16:25:59 ~5 min android-e2e 🤖apk 📲
✔️ df88fd2 #11 2023-10-23 16:26:06 ~5 min android 🤖apk 📲
✔️ df88fd2 #11 2023-10-23 16:26:47 ~6 min ios 📱ipa 📲
✔️ df88fd2 #12 2023-10-23 16:29:17 ~8 min tests 📄log
✔️ 8cefb3b #13 2023-10-26 14:47:28 ~6 min android 🤖apk 📲
✔️ 8cefb3b #13 2023-10-26 14:48:54 ~7 min android-e2e 🤖apk 📲
✔️ 8cefb3b #13 2023-10-26 14:49:28 ~8 min ios 📱ipa 📲
✔️ 8cefb3b #14 2023-10-26 14:50:51 ~9 min tests 📄log
✔️ ba294cb #14 2023-10-27 09:31:45 ~6 min android-e2e 🤖apk 📲
✔️ ba294cb #14 2023-10-27 09:31:52 ~6 min android 🤖apk 📲
✔️ ba294cb #14 2023-10-27 09:33:45 ~8 min ios 📱ipa 📲
✔️ ba294cb #15 2023-10-27 09:34:08 ~9 min tests 📄log
✔️ af7eba4 #15 2023-10-27 10:01:59 ~6 min android-e2e 🤖apk 📲
✔️ af7eba4 #15 2023-10-27 10:03:52 ~8 min android 🤖apk 📲
✔️ af7eba4 #15 2023-10-27 10:03:58 ~8 min ios 📱ipa 📲
✔️ af7eba4 #16 2023-10-27 10:05:32 ~9 min tests 📄log
✔️ 298e6a7 #16 2023-10-27 13:33:31 ~6 min android-e2e 🤖apk 📲
✔️ 298e6a7 #16 2023-10-27 13:33:43 ~6 min android 🤖apk 📲
✔️ 298e6a7 #16 2023-10-27 13:33:49 ~6 min ios 📱ipa 📲
✔️ 298e6a7 #17 2023-10-27 13:36:48 ~9 min tests 📄log
✔️ 84a2a79 #17 2023-10-30 22:58:10 ~5 min android 🤖apk 📲
✔️ 84a2a79 #17 2023-10-30 22:58:13 ~5 min ios 📱ipa 📲
✔️ 84a2a79 #17 2023-10-30 23:03:08 ~10 min android-e2e 🤖apk 📲
✔️ 84a2a79 #18 2023-10-30 23:03:13 ~10 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ fdbf9c2 #18 2023-11-01 10:22:05 ~5 min android-e2e 🤖apk 📲
✔️ fdbf9c2 #18 2023-11-01 10:24:17 ~7 min android 🤖apk 📲
✔️ fdbf9c2 #19 2023-11-01 10:26:06 ~9 min tests 📄log
✔️ fdbf9c2 #18 2023-11-01 10:28:17 ~11 min ios 📱ipa 📲
✔️ 30f3641 #19 2023-11-01 10:39:00 ~5 min android 🤖apk 📲
✔️ 30f3641 #19 2023-11-01 10:39:29 ~6 min ios 📱ipa 📲
✔️ 30f3641 #19 2023-11-01 10:41:01 ~7 min android-e2e 🤖apk 📲
✔️ 30f3641 #20 2023-11-01 10:43:01 ~9 min tests 📄log

@clauxx clauxx marked this pull request as ready for review October 18, 2023 23:16
Copy link
Contributor

@ajayesivan ajayesivan left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @clauxx 🎉

@OmarBasem
Copy link
Contributor

Hi @clauxx, thanks for your PR!

I have checked the builds and I noticed a bug. Pulling the text drawer up and down (without releasing touch) makes the images unscrollable. That's probably because the background opacity component is still displayed on top.

@OmarBasem
Copy link
Contributor

Hi @clauxx, thanks for your PR!

I have checked the builds and I noticed a bug. Pulling the text drawer up and down (without releasing touch) makes the images unscrollable. That's probably because the background opacity component is still displayed on top.

I noticed when dragging the text drawer down, it doesn't go down to the very end (part of the third line is visible). So that's why the background opacity component is still visible preventing scrolling

@ilmotta
Copy link
Contributor

ilmotta commented Oct 19, 2023

@clauxx, one thing that confused me happened a few times. In this video I scroll to the last line, and then I start pulling from the center side of the screen to scroll up, but as you can see, my gesture is sometimes detected to start closing the expanded text and then I need to scroll right on top of the text to not trigger the closing gesture.

scroll-behavior.webm

@clauxx
Copy link
Member Author

clauxx commented Oct 19, 2023

@clauxx, one thing that confused me happened a few times. In this video I scroll to the last line, and then I start pulling from the center side of the screen to scroll up, but as you can see, my gesture is sometimes detected to start closing the expanded text and then I need to scroll right on top of the text to not trigger the closing gesture.

scroll-behavior.webm

Ooof, this doesn't look right. Have to test it on android again. Thanks!

@clauxx
Copy link
Member Author

clauxx commented Oct 19, 2023

@clauxx, one thing that confused me happened a few times. In this video I scroll to the last line, and then I start pulling from the center side of the screen to scroll up, but as you can see, my gesture is sometimes detected to start closing the expanded text and then I need to scroll right on top of the text to not trigger the closing gesture.
scroll-behavior.webm

Ooof, this doesn't look right. Have to test it on android again. Thanks!

@ilmotta Weird. Tried on an android simulator and it looks fine (except for the missing gradient at the bottom, which will fix). Are you sure you're on the latest commit?

android.mp4

Copy link
Contributor

@ilmotta ilmotta left a comment

Choose a reason for hiding this comment

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

@clauxx, very good work in this PR! It feels smoother in my tests too when compared to develop.

About my bug report, I must have tested in the wrong branch or wrong revision because I could not reproduce again. Sorry to cause any trouble.

You are aware of the shadow issue, but I'm approving the PR in advance because I don't have anything else to add.

@clauxx
Copy link
Member Author

clauxx commented Oct 19, 2023

Hi @clauxx, thanks for your PR!

I have checked the builds and I noticed a bug. Pulling the text drawer up and down (without releasing touch) makes the images unscrollable. That's probably because the background opacity component is still displayed on top.

@OmarBasem Fixed the issue. Also removed the worklet for the text-sheet as it seems unnecessary, as the same can be achieved through an interpolation which makes it more readable (at least for me 😄), but let me know if it had a different purpose.

Copy link
Contributor

@OmarBasem OmarBasem left a comment

Choose a reason for hiding this comment

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

Good job! 🚀

@VolodLytvynenko
Copy link
Contributor

@clauxx Apologies. Hold on merging this PR after the design review, please. Need to rerun and investigate e2e one more time. I let you know if everything is alright

@VolodLytvynenko
Copy link
Contributor

@clauxx е2е failures are not related to this PR. Ready to be merged from my side again :) Thank you!

@clauxx
Copy link
Member Author

clauxx commented Oct 24, 2023

@clauxx е2е failures are not related to this PR. Ready to be merged from my side again :) Thank you!

@VolodLytvynenko Thank you!

@Francesca-G the PR is ready for design review

Copy link

@Francesca-G Francesca-G left a comment

Choose a reason for hiding this comment

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

Here's the Figma frame with the design review :)

@clauxx
Copy link
Member Author

clauxx commented Oct 26, 2023

@VolodLytvynenko @Francesca-G adjusted based on the design feedback

Copy link

@Francesca-G Francesca-G left a comment

Choose a reason for hiding this comment

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

Shadow looks good now 👍

Adding follow up required label because of the handle, as you can see it looks too visible compared to design:

Screenshot 2023-10-27 alle 09 22 40 Screenshot 2023-10-27 alle 09 22 50

@clauxx
Copy link
Member Author

clauxx commented Oct 27, 2023

Shadow looks good now 👍

Adding follow up required label because of the handle, as you can see it looks too visible compared to design:

Screenshot 2023-10-27 alle 09 22 40 Screenshot 2023-10-27 alle 09 22 50

@Francesca-G fixed it here, as it's not worth a follow-up issue for this small change. If I have your approval, I guess it can be merged, right @VolodLytvynenko?

@churik
Copy link
Member

churik commented Oct 27, 2023

@clauxx merge it!

fix: improving UI and gestures

fix: expanding on press now works reliably

fix: center text and disable gestures for short messages

feat: added styling for one-lined messages

fix: small opacity fix

ref: small condition change

fix: horizontal swiping being unreliable with vertical scrolling

ref: reusing shared value from lightbox animations

fix: text-sheet bottom gradient not visible on android

fix: addressed qa issues 1,2,4

fix: zoomable-image stuck if moved vertically when full screen

clean: removed unused var

chore: removed unused require

fix: don't allow returning text-sheet down and up in one move

fix: text-sheet bar styling was wrong

fix: adjusted gradient animation with designs

fix: text-sheet bar opacity
@clauxx
Copy link
Member Author

clauxx commented Nov 1, 2023

@VolodLytvynenko there's small issue with the commits you co-authored. They're not verified, as in the commits are not signed, so they can't be merged. There's a doc on how to set it up, but if you have questions or need help with that let me know. I fixed it here by hand-picking the commits, but it's easier for the future if all commits are verified when co-authoring.

@clauxx clauxx merged commit 716007d into develop Nov 1, 2023
6 checks passed
@clauxx clauxx deleted the 17555-chat-image-shadow branch November 1, 2023 11:20
@VolodLytvynenko
Copy link
Contributor

@VolodLytvynenko there's small issue with the commits you co-authored. They're not verified, as in the commits are not signed, so they can't be merged. There's a doc on how to set it up, but if you have questions or need help with that let me know. I fixed it here by hand-picking the commits, but it's easier for the future if all commits are verified when co-authoring.

Hi @clauxx just added the GPG keys. Thanx!

yevh-berdnyk pushed a commit that referenced this pull request Dec 8, 2023
fix: improving UI and gestures

fix: expanding on press now works reliably

fix: center text and disable gestures for short messages

feat: added styling for one-lined messages

fix: small opacity fix

ref: small condition change

fix: horizontal swiping being unreliable with vertical scrolling

ref: reusing shared value from lightbox animations

fix: text-sheet bottom gradient not visible on android

fix: addressed qa issues 1,2,4

fix: zoomable-image stuck if moved vertically when full screen

clean: removed unused var

chore: removed unused require

fix: don't allow returning text-sheet down and up in one move

fix: text-sheet bar styling was wrong

fix: adjusted gradient animation with designs

fix: text-sheet bar opacity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

Unwanted shadow effect when scrolling image description (IOS)
9 participants