[#21227] Simpler chat screen #21313
[#21227] Simpler chat screen #21313
Conversation
ulisesmac
requested review from
ilmotta,
seanstrom,
flexsurfer,
Parveshdhull and
clauxx
| (defn view | ||
| [{:keys [layout-height]}] | ||
| [layout-height] | ||
| (let [suggestions (rf/sub [:chat/mention-suggestions]) |
There was a problem hiding this comment.
We passed a big map of props but we only used one of them, which is actually an atom, so it's a stable reference, no fear from uinexpected re-renders now. This is a simplification made.
| (def ^:const pinned-banner-height 40) | ||
| (def ^:const header-container-top-margin 48) | ||
| (def ^:const header-container-radius 20) | ||
| (def ^:const header-animation-distance 20) | ||
| (def ^:const content-animation-start-position-android 130) | ||
| (def ^:const content-animation-start-position-ios 124) | ||
| ;; Note - We should also consider height of bio for banner animation starting position | ||
| ;; Todo - Should be updated once New-profile implemation is complete | ||
| (def ^:const pinned-banner-animation-start-position 148) | ||
|
|
||
| (def ^:const default-extrapolation-option | ||
| {:extrapolateLeft "clamp" | ||
| :extrapolateRight "clamp"}) |
There was a problem hiding this comment.
Now unused consts, some animations were no longer necessary.
| (ns status-im.contexts.chat.messenger.placeholder.style | ||
| (:require [quo.foundations.colors :as colors] | ||
| [react-native.reanimated :as reanimated])) | ||
| [react-native.safe-area :as safe-area])) | ||
|
|
||
| (defn container | ||
| [top opacity z-index theme] | ||
| (reanimated/apply-animations-to-style | ||
| {:opacity opacity | ||
| :z-index z-index} | ||
| {:position :absolute | ||
| :padding-top top | ||
| :top 0 | ||
| :left 0 | ||
| :right 0 | ||
| :bottom 0 | ||
| :background-color (colors/theme-colors colors/white colors/neutral-95 theme)})) | ||
| [theme on-layout-done?] | ||
| {:position :absolute | ||
| :padding-top (safe-area/get-top) | ||
| :top 0 | ||
| :left 0 | ||
| :right 0 | ||
| :bottom 0 | ||
| :background-color (colors/theme-colors colors/white colors/neutral-95 theme) | ||
| :opacity (if on-layout-done? 0 1) | ||
| :z-index (if on-layout-done? 0 2)}) |
There was a problem hiding this comment.
We had some shared values to control the logic here, but we didn't do any animations, and a ratom can do the job, (this is the piece of code I mentioned works simpler with a ratom)
| (when *screen-loaded?* | ||
| (rn/use-mount #(reset! *screen-loaded?* true))) |
There was a problem hiding this comment.
We conditionally used a hook
Jenkins BuildsClick to see older builds (12)
|
| (when-not (if *screen-loaded?* @*screen-loaded?* screen-loaded?) | ||
| (reanimated/set-shared-value chat-screen-layout-calculations-complete? false) | ||
| (reanimated/set-shared-value distance-from-list-top 0) | ||
| (reanimated/set-shared-value chat-list-scroll-y 0)) | ||
| (when (if *screen-loaded?* @*screen-loaded?* screen-loaded?) |
There was a problem hiding this comment.
when + if seems hard to read
| *screen-loaded?* (when-not jump-to-enabled? | ||
| (reagent/atom false))] |
There was a problem hiding this comment.
ratom being created in a non-form-2-component. This works because we aren't re-rendering this component, but as soon as ff/enabled? changes during this screen mounted, it won't work.
Although it works now, I prefered to refactor to properly create the ratom.
| (let [screen-loaded? (rf/sub [:shell/chat-screen-loaded?]) | ||
| distance-from-list-top (reanimated/use-shared-value 0) | ||
| chat-list-scroll-y (reanimated/use-shared-value 0) | ||
| props {:insets (safe-area/get-insets) | ||
| :content-height (atom 0) | ||
| :layout-height (atom 0) | ||
| :distance-atom (atom 0) | ||
| :distance-from-list-top distance-from-list-top | ||
| :chat-list-scroll-y chat-list-scroll-y | ||
| :chat-screen-layout-calculations-complete? | ||
| chat-screen-layout-calculations-complete?}] |
There was a problem hiding this comment.
These atoms are not created in a form-2 component, I moved them to be use-ref-atom.
| (reset! layout-height layout-height-new)) | ||
| (when-not (reanimated/get-shared-value chat-screen-layout-calculations-complete?) | ||
| (reanimated/set-shared-value chat-screen-layout-calculations-complete? true)))) |
There was a problem hiding this comment.
The layout calculation is just a boolean marked once the on-layout event was fired. It's been simplified to only be a ratom switch.
| (let [content-height (rn/use-ref-atom 0) | ||
| distance-atom (rn/use-ref-atom 0) |
There was a problem hiding this comment.
These two values were received, but there's no need to lift up the state even more, they are only used here, so they are being created here.
| :customization-color customization-color}] | ||
| :footer [more-messages-loader | ||
| {:chat-id chat-id | ||
| :window-height window-height}] | ||
| :data messages |
There was a problem hiding this comment.
unrelated to the changes made in this PR, but since the chat screen is getting reworked, we could also consider the way we do the rendering in the chat. Currently we pass a list of messages as data, which makes it slower when the flat-list re-renders (and it will quite often). For large lists we should consider just passing a list of "message ids" and get the message data through a subscription inside the list items.
There was a problem hiding this comment.
I'd really like to check the rendering for the chat screen and improve the implementation to make it more optimal 🙂
As I said in another response, we could file an issue to check this and improve it. Addressing it in this PR would make the review even harder.
CC: @ilmotta
There was a problem hiding this comment.
Nice suggestion @clauxx, this can work well, but worth measuring first to be sure it's worth it.
As I said in another response, we could file an issue to check this and improve it. Addressing it in this PR would make the review even harder.
This is the key for us to keep develop releasable. PRs with focused scope and with clear requirements for QAs and devs to verify. Performance optimizations are much easier to test in isolation.
| :content-container-style {:padding-top distance-from-last-message | ||
| :padding-bottom top-margin} |
There was a problem hiding this comment.
Should be in styles based on the guidelines, but don't mind it tbh
| (let [on-layout-done? (reagent/atom false) | ||
| first-render-done? (reagent/atom false)] |
There was a problem hiding this comment.
Could the on-layout-done? ratom be avoided smh? Like having a value in the db for it maybe or smh else, or does that make it much slower to open?
Asking cause it's passed through a few children, mutated "somewhere" and used in another "somewhere", so I wonder about the complexity trade-offs you mentioned, if any.
There was a problem hiding this comment.
Thanks for this comment and for noticing it!
I know this ratom is being passed, but we need to use the advantages of being a ratom and deref it in its children.
It can't be substituted with a hook (unless we use the js/requestAnimationFrame to update its value).
It could be moved to re-frame, but I think there's no need to move this local state to be a global one.
This ratom was originally called something like chat-screen-layout-calculations-complete, and what I did was rename it and simplify the code, but I preserved the same main logic, so it means the behavior is the same.
I can rework the logic and bring a simpler one, but I didn't want to change it because the issue solved is mostly about a redesign.
I can open a new issue with a list of code improvements we want to fix in the chat screen and then solve it. Also, the review will be easier. wdyt?
There was a problem hiding this comment.
It could be moved to re-frame, but I think there's no need to move this local state to be a global one.
Since it's mutated and derefed in nested children, I'd argue it doesn't/shouldn't fall under local state anymore, but I agree that this will introduce even more changes and could be should outside this PR 👍.
There was a problem hiding this comment.
Wow, when this simplification was designed in Figma I had no idea we would be able to clean up so much. Thank you @ulisesmac
ulisesmac
force-pushed
the
21227-Simpler-chat
branch
from
161fc54 to
de24093
Compare
VolodLytvynenko
force-pushed
the
21227-Simpler-chat
branch
from
de24093 to
b95f5a9
Compare
84% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (43)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
100% of end-end tests have passedPassed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
Hey @ulisesmac ! Thanks for your PR. Looks good on both platforms and PR is ready to be merged. |
….messenger.messages.list.view` to be more REPL friendly
- Change shared-value to ratom usage since no animations were performed.
The previous implementation has two main problems: 1. It is creating atoms and ratoms in non-form-2-components. 2. Conditionally uses a hook (`rn/use-mount` in `lazy-chat-screen`). The new implementation fixes the previous problems, improves the readability and passes only the props needed to children components.
ulisesmac
force-pushed
the
21227-Simpler-chat
branch
from
103f561 to
2d7ce71
Compare
fixes #21227
fixes #20680
Summary
This PR implements the new simpler designs for the chat screen.
Lot of changes were made since some additional (but need) improvements were made for the sake of readability, but it preserves the past behavior (besides the redesign).
Review notes
The implementation is keeping a couple of ratoms in the
chatcomponent due to their "async renderer" nature, the same result could be achieved by React hooks, but I'd need to usejs/requestAnimationFramealong with ause-effectwhich makes the code more complex.The usage of ratoms is to avoid a blank screen while the chat screen is opened:
with ratoms:
with-atom.mp4
with hooks:
with-hooks.mp4
Please refer to the self-review comments if you want to explore the implementation details more deeply.
Testing notes
Please test that the chat screen is behaving in the same way as before for 1:1 chats, group chats and channels, and it only changes the design.
Platforms
Functional
status: ready