-
Notifications
You must be signed in to change notification settings - Fork 984
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
Conversation
Jenkins BuildsClick to see older builds (53)
|
13b3dcd
to
694bb7e
Compare
There was a problem hiding this 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 🎉
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 |
@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 |
There was a problem hiding this 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.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 🚀
@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 |
@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 |
There was a problem hiding this 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 :)
@VolodLytvynenko @Francesca-G adjusted based on the design feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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? |
@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
fdbf9c2
to
30f3641
Compare
@VolodLytvynenko there's small issue with the commits you co-authored. They're not |
Hi @clauxx just added the GPG keys. Thanx! |
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
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:
@OmarBasem since you worked on it originally, please have a look to make sure I didn't miss/break anything.
Testing notes
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-10-19.at.02.04.18.mp4
status: ready