-
Notifications
You must be signed in to change notification settings - Fork 985
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 android composer background shadow not visible when image is attached #19492
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,19 +70,17 @@ | |
|
||
(defn audio-effect | ||
[{:keys [recording? gesture-enabled?]} | ||
{:keys [container-opacity]} | ||
audio] | ||
(when (and audio (not @recording?)) | ||
(reset! recording? true) | ||
(reset! gesture-enabled? false) | ||
(reanimated/animate container-opacity 1))) | ||
(reset! gesture-enabled? false))) | ||
|
||
(defn empty-effect | ||
[{:keys [focused?]} | ||
{:keys [container-opacity]} | ||
[{:keys [empty-input?]} | ||
{:keys [input-text images link-previews? reply audio]}] | ||
(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))) | ||
|
||
(defn component-will-unmount | ||
[{:keys [keyboard-show-listener keyboard-hide-listener keyboard-frame-listener]}] | ||
|
@@ -100,8 +98,8 @@ | |
(kb-default-height-effect state) | ||
(background-effect state animations dimensions chat-input) | ||
(link-preview-effect state) | ||
(audio-effect state animations audio) | ||
(empty-effect state animations subscriptions) | ||
(audio-effect state audio) | ||
(empty-effect animations subscriptions) | ||
(kb/add-kb-listeners props state animations dimensions) | ||
#(component-will-unmount props)) | ||
[max-height]) | ||
|
@@ -160,13 +158,10 @@ | |
|
||
(defn use-reply | ||
[{:keys [input-ref]} | ||
{:keys [container-opacity]} | ||
{:keys [reply]} | ||
chat-screen-layout-calculations-complete?] | ||
(rn/use-effect | ||
(fn [] | ||
(when reply | ||
(reanimated/animate container-opacity 1)) | ||
(when (and reply @input-ref (reanimated/get-shared-value chat-screen-layout-calculations-complete?)) | ||
(js/setTimeout #(.focus ^js @input-ref) 600))) | ||
[(:message-id reply)])) | ||
|
@@ -203,12 +198,10 @@ | |
(defn use-images | ||
[{:keys [sending-images? input-ref]} | ||
{:keys [text-value maximized?]} | ||
{:keys [container-opacity height saved-height]} | ||
{:keys [height saved-height]} | ||
{:keys [images]}] | ||
(rn/use-effect | ||
(fn [] | ||
(when images | ||
(reanimated/animate container-opacity 1)) | ||
Comment on lines
-210
to
-211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Double checked just now, working fine. Thank you for reminding. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also checked this case and it looks fine for me |
||
(when (and (not @sending-images?) (seq images) @input-ref) | ||
(.focus ^js @input-ref)) | ||
(if-not @maximized? | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ | |
(defn drag-gesture | ||
[{:keys [input-ref] :as props} | ||
{:keys [gesture-enabled?] :as state} | ||
{:keys [height saved-height last-height opacity background-y container-opacity] :as animations} | ||
{:keys [height saved-height last-height opacity background-y] :as animations} | ||
{:keys [max-height lines] :as dimensions} | ||
keyboard-shown] | ||
(let [expanding? (atom true) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also here, is the container's opacity being handled somewhere else on-focus? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
(reanimated/set-shared-value last-height max-height) | ||
(maximize state animations dimensions)) | ||
(when @input-ref | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,8 @@ | |
(defn focus | ||
"Animate to the `saved-height`, display background-overlay if needed, and set cursor position" | ||
[{:keys [input-ref] :as props} | ||
{:keys [text-value focused? lock-selection? saved-cursor-position composer-focused? maximized?]} | ||
{:keys [height saved-height last-height opacity background-y container-opacity] | ||
{:keys [text-value focused? lock-selection? saved-cursor-position maximized?]} | ||
{:keys [height saved-height last-height opacity background-y composer-focused?] | ||
:as animations} | ||
{:keys [max-height] :as dimensions}] | ||
(reanimated/set-shared-value composer-focused? true) | ||
|
@@ -27,7 +27,6 @@ | |
new-height (min max-height last-height-value)] | ||
(reanimated/animate height new-height) | ||
(reanimated/set-shared-value saved-height new-height) | ||
(reanimated/animate container-opacity 1) | ||
(when (> last-height-value (* constants/background-threshold max-height)) | ||
(reset! maximized? true) | ||
(reanimated/animate opacity 1) | ||
|
@@ -42,10 +41,9 @@ | |
(defn blur | ||
"Save the current height, minimize the composer, animate-out the background, and save cursor position" | ||
[{:keys [text-value focused? lock-selection? cursor-position saved-cursor-position gradient-z-index | ||
maximized? recording? composer-focused?]} | ||
{:keys [height saved-height last-height gradient-opacity container-opacity opacity background-y]} | ||
{:keys [content-height max-height window-height]} | ||
{:keys [images link-previews? reply]}] | ||
maximized? recording?]} | ||
{:keys [height saved-height last-height gradient-opacity opacity background-y composer-focused?]} | ||
{:keys [content-height max-height window-height]}] | ||
(when-not @recording? | ||
(let [lines (utils/calc-lines (- @content-height constants/extra-content-offset)) | ||
min-height (utils/get-min-height lines) | ||
|
@@ -62,8 +60,6 @@ | |
(reanimated/set-shared-value saved-height min-height) | ||
(reanimated/animate opacity 0) | ||
(js/setTimeout #(reanimated/set-shared-value background-y (- window-height)) 300) | ||
(when (utils/empty-input? @text-value images link-previews? reply nil) | ||
(reanimated/animate container-opacity constants/empty-opacity)) | ||
(reanimated/animate gradient-opacity 0) | ||
(reset! lock-selection? true) | ||
(reset! saved-cursor-position @cursor-position) | ||
|
@@ -164,8 +160,3 @@ | |
(let [{:keys [start end text-input-handle]} @selection-event] | ||
(selection/update-selection text-input-handle start end) | ||
(reset! selection-event nil))))) | ||
|
||
(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")))) | ||
Comment on lines
-167
to
-171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not need this anymore? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,12 @@ | |
(def border-top-radius 20) | ||
|
||
(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)}})) | ||
Comment on lines
12
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have checked those cases, shadow looks okay to me |
||
|
||
(def composer-sheet-and-jump-to-container | ||
{:position :absolute | ||
|
@@ -25,17 +24,18 @@ | |
:right 0}) | ||
|
||
(defn sheet-container | ||
[insets {:keys [focused?]} {:keys [container-opacity]} theme] | ||
[insets {:keys [container-opacity composer-elevation]} theme] | ||
(reanimated/apply-animations-to-style | ||
{:opacity container-opacity} | ||
{:opacity container-opacity | ||
:elevation composer-elevation} | ||
(merge | ||
{:border-top-left-radius border-top-radius | ||
:border-top-right-radius border-top-radius | ||
:padding-horizontal 20 | ||
:background-color (colors/theme-colors colors/white colors/neutral-95 theme) | ||
:z-index 3 | ||
:padding-bottom (:bottom insets)} | ||
(shadow focused? theme)))) | ||
(shadow theme)))) | ||
|
||
(def bar-container | ||
{:height constants/bar-container-height | ||
|
@@ -95,17 +95,16 @@ | |
:background-color colors/neutral-95-opa-70})) | ||
|
||
(defn blur-container | ||
[height focused?] | ||
(reanimated/apply-animations-to-style | ||
{:height height} | ||
[composer-default-height {:keys [blur-container-elevation]}] | ||
[{:elevation blur-container-elevation} | ||
{:position :absolute | ||
:elevation (if-not @focused? 10 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we removing the shadow completely on Android? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, moved to |
||
:left 0 | ||
:right 0 | ||
:bottom 0 | ||
:height composer-default-height | ||
:border-top-right-radius border-top-radius | ||
:border-top-left-radius border-top-radius | ||
:overflow :hidden})) | ||
:overflow :hidden}]) | ||
|
||
(defn blur-view | ||
[theme] | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 incomposerContainerOpacity
in thecomposer.js
file.