fix android composer background shadow not visible when image is attached #19492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (8)
|
94% of end-end tests have passedFailed tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
pavloburykh
force-pushed
the
fix/composer-photo-shadow
branch
from
99eb361 to
76ab1d5
Compare
pavloburykh
force-pushed
the
fix/composer-photo-shadow
branch
from
76ab1d5 to
d080259
Compare
|
@OmarBasem @alwx hello guys! Could please someone review this PR? Thank you. |
ibrkhalil
approved these changes
Apr 8, 2024
OmarBasem
approved these changes
Apr 8, 2024
OmarBasem
reviewed
Apr 8, 2024
Comment on lines
-84
to
+83
| (when (and (not @focused?) (utils/empty-input? input-text images link-previews? reply audio)) | ||
| (reanimated/animate-delay container-opacity constants/empty-opacity 200))) | ||
| (reanimated/set-shared-value | ||
| empty-input? | ||
| (utils/empty-input? input-text images link-previews? reply audio))) |
There was a problem hiding this comment.
This was an animation, and now it is setting a shared value. Are you sure about this change @Parveshdhull ?
There was a problem hiding this comment.
hi @OmarBasem, Thank you very much for reviewing PR.
Yes, the empty-input? shared value is supposed to be updated whenever we have input empty without delay. We need animation for container-opacity, which is now handled in composerContainerOpacity in the composer.js file.
Comment on lines
-210
to
-211
| (when images | ||
| (reanimated/animate container-opacity 1)) |
There was a problem hiding this comment.
Is the container's opacity being animated on adding images somewhere else? (and have you tested adding images, then existing and entering the chat screen)
There was a problem hiding this comment.
Yes, as I mentioned in above comment all animations for container-opacity is handled in composer.js file at one place. Now we don't need to handle this manually.
and have you tested adding images, then existing and entering the chat screen
Double checked just now, working fine. Thank you for reminding.
There was a problem hiding this comment.
I also checked this case and it looks fine for me
OmarBasem
reviewed
Apr 8, 2024
| @@ -68,7 +68,6 @@ | |||
| (if-not keyboard-shown | |||
| (do ; focus and end | |||
| (when (< (oops/oget event "velocityY") constants/velocity-threshold) | |||
| (reanimated/set-shared-value container-opacity 1) | |||
There was a problem hiding this comment.
Also here, is the container's opacity being handled somewhere else on-focus?
OmarBasem
reviewed
Apr 8, 2024
Comment on lines
-167
to
-171
|
|
||
| (defn layout | ||
| [event state blur-height] | ||
| (when (utils/update-blur-height? event state blur-height) | ||
| (reanimated/set-shared-value blur-height (oops/oget event "nativeEvent.layout.height")))) |
There was a problem hiding this comment.
Do we not need this anymore?
There was a problem hiding this comment.
Composer is only blurred when it is collapsed and empty, not sure why we animated blur height in first place. But yes we don't this anymore
OmarBasem
reviewed
Apr 8, 2024
Comment on lines
12
to
+18
| (defn shadow | ||
| [focused? theme] | ||
| (if platform/ios? | ||
| [theme] | ||
| (when platform/ios? | ||
| {:shadow-radius 20 | ||
| :shadow-opacity (colors/theme-colors 0.1 0.7 theme) | ||
| :shadow-color colors/neutral-100 | ||
| :shadow-offset {:width 0 :height (colors/theme-colors -4 -8 theme)}} | ||
| {:elevation (if @focused? 10 0)})) | ||
| :shadow-offset {:width 0 :height (colors/theme-colors -4 -8 theme)}})) |
There was a problem hiding this comment.
Is the elevation here not needed? Does the shadow look right with all combinations of being focused and not focused, and having content and not having content?
There was a problem hiding this comment.
Yes, it looks fine. But just to be double sure, @pavloburykh Please keep an eye on this while testing.
There was a problem hiding this comment.
I have checked those cases, shadow looks okay to me
OmarBasem
reviewed
Apr 8, 2024
| {:position :absolute | ||
| :elevation (if-not @focused? 10 0) |
There was a problem hiding this comment.
Are we removing the shadow completely on Android?
There was a problem hiding this comment.
Nope, moved to blur-container-elevation
|
@OmarBasem thanks for the review. @Parveshdhull please, ping me up when review comments will be addressed. I have tested the PR and it looks good to me. If there will be any changes based on the review please let me know so I perform re-test. Thank you. |
|
@Parveshdhull if not changes are required - PR is ready for merge. |
Parveshdhull
force-pushed
the
fix/composer-photo-shadow
branch
from
d080259 to
99eb361
Compare
Parveshdhull
force-pushed
the
fix/composer-photo-shadow
branch
from
99eb361 to
887395e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18953
Summary
PR fixes the composer shadow problem, also as bonus makes blur view static instead of animated one.
Testing notes:
status: ready