[#21436] List of users overlap composer input #21641
Conversation
| :style {:height "100%" | ||
| :background-color (colors/theme-colors colors/white |
There was a problem hiding this comment.
This is the layout fix for the reported issue
| (defn- add-shadow | ||
| [theme styles] | ||
| (cond-> styles | ||
| platform/ios? (assoc :shadow-radius (colors/theme-colors 30 50 theme) | ||
| :shadow-opacity (colors/theme-colors 0.1 0.7 theme) | ||
| :shadow-color colors/neutral-100 | ||
| :shadow-offset {:width 0 :height (colors/theme-colors 8 12 theme)}) | ||
| :else (assoc :elevation 10))) |
There was a problem hiding this comment.
Modified this to use assoc instead of merge 👀
There was a problem hiding this comment.
Here I think cond-> is more convoluted to read than a simple if macro.
(defn- add-shadow
[theme styles]
(if platform/ios?
(assoc styles
:shadow-radius (colors/theme-colors 30 50 theme)
:shadow-opacity (colors/theme-colors 0.1 0.7 theme)
:shadow-color colors/neutral-100
:shadow-offset {:width 0 :height (colors/theme-colors 8 12 theme)})
(assoc styles :elevation 10)))There was a problem hiding this comment.
Haha 😅 I completely agree! I'll update it 👍 Thanks
| (defn container | ||
| [opacity top theme] | ||
| (reanimated/apply-animations-to-style | ||
| {:opacity opacity} | ||
| (merge | ||
| {:position :absolute | ||
| :top (- (+ 8 top)) | ||
| :left 8 | ||
| :right 8 | ||
| :border-radius 16 | ||
| :z-index 4 | ||
| :max-height constants/mentions-max-height | ||
| :background-color (colors/theme-colors colors/white colors/neutral-95 theme)} | ||
| (shadow theme)))) | ||
| [{:opacity opacity} | ||
| (add-shadow theme | ||
| {:position :absolute | ||
| :top (- (+ 8 top)) | ||
| :left 8 | ||
| :right 8 | ||
| :border-radius 16 | ||
| :z-index 4 | ||
| :max-height constants/mentions-max-height | ||
| :background-color (colors/theme-colors colors/white colors/neutral-95 theme)})]) |
There was a problem hiding this comment.
I tried several ways to solve the rendering issue in the mentions component, but nothing else worked.
The problem is caused by our apply-animations-to-style wrapper, seems its usage delays the rendering of its stytles ~1 frame.
By avoiding its usage, we get the component state updated on time, here we are using the vector syntax introduced a while ago.
CC: @ilmotta
Jenkins BuildsClick to see older builds (1)
|
ulisesmac
force-pushed
the
21436-list-of-users-overlap-input
branch
from
f916a62
to
dc532cd
Compare
| [rn/flat-list | ||
| [suggestions]) | ||
| [reanimated/view {:style (style/container opacity top theme)} | ||
| [reanimated/flat-list |
There was a problem hiding this comment.
What do we gain by using a flat list here instead of the macro for? I checked with a simple for and it all works the same apparently, including the animation.
Related to your comment #21641 (comment), we should consider removing micro animation code in certain places if they get in our way during development at this stage of the product. I'm not saying we should always do that with micro animations. Here, you fought a battle related to state synchronization between two sources as you and I talked in private. This view component contains workarounds that make the code brittle.
The code below is much simpler and delivers the same experience in my opinion. Or perhaps my suggested code has a flaw, but I couldn't see.
(defn view
[layout-height]
(let [suggestions (rf/sub [:chat/mention-suggestions])
theme (quo.theme/use-theme)
top (min constants/mentions-max-height
(* (count suggestions) 56)
(- @layout-height
(+ (safe-area/get-top)
messages.constants/top-bar-height
5)))]
[rn/view {:style (style/container top theme)}
(for [user (vals suggestions)]
^{:key (:key user)}
[mention-item user])]))Anyway, this is just a light suggestion to remove the micro animation to simplify, but since you solved the problem it's okay to keep the animation @ulisesmac.
There was a problem hiding this comment.
What do we gain by using a flat list here instead of the macro for? I checked with a simple for and it all works the same apparently, including the animation.
I think without the flatlist we wouldn't get the scroll if the list is large, but unsure if we want that. We could limit the amount of results we show also if we don't want the scroll, might make it lighter to re-render.
There was a problem hiding this comment.
What do we gain by using a flat list here instead of the macro for? I checked with a simple for and it all works the same apparently, including the animation.
@ilmotta A flat-list is lazy, it's built to render a lot of items, it just renders what you see (window-size) and some windows above and below (it's configurable). Having a for inside a scroll-view (to keep the scrolling) will render all possible suggestions at once. Not the best for this use case.
There was a problem hiding this comment.
@ilmotta
Thanks for your simplification. I think the opacity animation gives a slightly smoother experience, ofc, I'd like to improve the code by removing that use-effect, but I think we will need this hook in any animated component. Also, since the problem is solved I'd prefer to keep it.
We can explore ways to make this cleaner later, since in the future we will add animations to our app to make its usage more enjoyable.
There was a problem hiding this comment.
The list of mentions is limited by 10 items I think.
There was a problem hiding this comment.
In which case the flatlist is unnecessary. #21131
There was a problem hiding this comment.
Cool, then we can use a scrollview! I'll change it.
fixes #21436
Summary
This PR fixes the styles so that the input is no longer covered.
While solving this, I noticed we were updating the mention's state asynchronously, and it looked as follows:
bug.mp4
Screenshots (to check how the mentions are aligned):
Not it's been fixed and looks as follows:
solved.mp4
Review notes
Testing notes
Platforms
Steps to test
status: ready