New mentions design #15799
Merged
New mentions design #15799
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 (12)
|
qfrank
reviewed
May 3, 2023
qfrank
approved these changes
May 3, 2023
qfrank
reviewed
May 3, 2023
ilmotta
reviewed
May 3, 2023
ajayesivan
approved these changes
May 3, 2023
ilmotta
approved these changes
May 3, 2023
There was a problem hiding this comment.
Thanks @OmarBasem, this is my last round of review here. LGTM
For future iterations, it would be good to optimize the way mentions are being processed and rendered.
- Every pressed key causes all the mention logic to be triggered (including events). The text input is being parsed too frequently by
status-go, and this causes a considerable overhead in the rendering pipeline. Enable the debug logs to see. This existed before your PR, so I'm just sharing if you didn't know. This is VERY expensive. We should protect high frequency events with something similar toutils.debouce/debounce-and-dispatch. You might be interested in this @qfrank, it's quite sensible to fix. - The mention items on the FlatList at the top of the chat are being re-rendered on every key press after
@, even though they didn't change (from the user's perspective). - This one I mentioned in another thread in this PR, but we're double-rendering
f-viewon every key press due to the duplicated statesuggestions-atom. Maybe there's a way to fix this while still retaining the animation behavior, but I have no practical solution to share. - The mention suggestions position is being recalculated on every key press after
@, even though it's position is the same. We should memoize the calculation. Again another good reason to keep these kinds of functions pure, since memoization is only reliable on pure functions.
So I'd say it's worth in another PR to attack these problems because we need the chat input to be as smooth as we can make it.
src/status_im2/contexts/chat/bottom_sheet_composer/mentions/view.cljs
Outdated
Show resolved
Hide resolved
|
|
||
| (defn view | ||
| [props state animations max-height cursor-pos] | ||
| (let [suggestions-atom (reagent/atom {})] |
There was a problem hiding this comment.
This atom doesn't need to live here, since it's only used by f-view.
There was a problem hiding this comment.
moving it to f-view causes the suggestions list not to appear when typing '@'
review lint review udpates updates updates udpates updates updates lint updates composer shell button updates updates updates updates updates updates updates updates updates updates updates updates updates
OmarBasem
force-pushed
the
new-mentions-design
branch
from
4e5ad86 to
9268060
Compare
|
@pavloburykh as I informed you yesterday we will do testing on the final PR. I need to merge this one now for dependency. |
@OmarBasem yeah, I remember. But I see that e2e has not been automatically triggered for this PR so I have launched e2e run manually. |
|
@pavloburykh Oh okay, I will wait for e2e |
I will let you know once e2e run is finished |
Thanks @ilmotta for your review. I will consider further performance optimizations for another PR. |
83% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (25)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@OmarBasem e2e run has finished. I believe failed e2e are related to the new composer changes. We will address those issues in the final PR as discussed. One question: is this expected that mention list in current PR does not correspond to new mentions design? Actual result:
Expected result:
|
|
@pavloburykh that's from the old composer. The new design is implemented but still disabled. Feel free to log any issues from e2e here: #15747 |
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: #15331
This PR implements mentions new UI design for the new composer.
Note: the new composer is still a work in progress and is still disabled on develop. Sill one more issue to go: #15747