Highlight mentions in chat input #16330
Conversation
Jenkins BuildsClick to see older builds (69)
|
| [input-with-mentions] | ||
| (reduce (fn | ||
| [styled-text [chunk-type chunk-content]] | ||
| (cond |
There was a problem hiding this comment.
(condp = chunk-type
:text (conj..
:mention (conj ...)
Avoids case as it's not safe and it's a bit more compact than just cond
vkjr
force-pushed
the
mentions_not_resolved
branch
from
8f4ed5b
to
b6ca94a
Compare
vkjr
changed the title
| input-text)}}))) | ||
| 300))) | ||
| [(some #(= :mention (first %)) (seq input-with-mentions))])) | ||
| #_(defn edit-mentions |
There was a problem hiding this comment.
probably you should include a TODO here with a link for the issue to restore this functionality 👍
| [quo2.theme :as theme])) | ||
| [quo2.theme :as theme] | ||
| [quo2.core :as quo])) | ||
|
|
| (effects/update-input-mention props state subs) | ||
| (effects/edit-mentions props state subs) | ||
| (effects/update-input-mention state subs) | ||
| #_(effects/edit-mentions props state subs) |
There was a problem hiding this comment.
@J-Son89, it is "wip" PR, created for the sake of getting ios builds for device) Don't need a review yet, since it is not a final version, there is still a bug that I'm trying to solve) All code pieces that are not needed will be removed of course, not commented)
There was a problem hiding this comment.
Apologies, I missed that. Please ignore my comments so 😅
vkjr
changed the title
vkjr
force-pushed
the
mentions_not_resolved
branch
from
93b334a
to
cd47b59
Compare
| (if (string/ends-with? text "@") | ||
| (rf/dispatch [:mention/on-change-text text]) | ||
| (debounce/debounce-and-dispatch [:mention/on-change-text text] 300))) | ||
| (rf/dispatch [:mention/on-change-text text])) |
There was a problem hiding this comment.
Why this change? I think this is to avoid many calls to status-go for mentions cc @qfrank
There was a problem hiding this comment.
Also would like to know why 🙂
There was a problem hiding this comment.
@OmarBasem, @qfrank, two things:
-
we need to get updated text structure from
status-goas quick as possible, because while it is not updated, highlighting is not applied -
With previous logic I experienced a bug. Imagine that user is going to type "hello @". The order of event is:
- user types "hello"
- user types whitespace after "hello",
on-change-textscheduled with delay and with content "hello " - user types "@",
on-change-textimmediately processed and current text input becomes "hello @" - scheduled
on-change-textprocessed with delay and current text get backs to the "hello ", which results in losing the "@"
There was a problem hiding this comment.
emm.. what if we enhance debounce/debounce-and-dispatch to make it support dispatch immediately if user input end with @ ? @vkjr
There was a problem hiding this comment.
@qfrank, I think debounce-and-dispatch is too general to make this change, because it works not only with on-change-text, but even if we add this hack, that won't eliminate the problem with slow highlight updating.
Imagine that user typed @asd and continued quick typing. All the symbols after "@" won't be highlighted because while user quickly types, events not sent and updated text structure is not received from status-go
There was a problem hiding this comment.
yeah, quick typing would make weired behavior. 😅
debounce-and-dispatch is too general
what if make a new function like debounce-and-dispatch and add an extra trigger condition to the actual invoking e.g. the number of user input characters reached a specific value. just my thoughts , it's up to you, if we found performance issue with mention, there still exist spaces to improve in status-go side 🙂
There was a problem hiding this comment.
@qfrank, I think right now it is more a preliminary optimization) Let's see if there are any issues with existing implementation and then start improving, wdyt? I'm currently more concerned about that bug with highlighting that is in the description. I believe it can be a showstopper on slow devices :-/
vkjr
force-pushed
the
mentions_not_resolved
branch
from
cd47b59
to
3d31249
Compare
79% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (26)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
vkjr
force-pushed
the
mentions_not_resolved
branch
from
3d31249
to
be357b8
Compare
|
@pavloburykh, there is no additional functionality, I just rebased to the latest develop before figuring out there is a blocker :-/ Testing yesterday's build is fine, no changes since then) |
Thank you @vkjr ! |
vkjr
force-pushed
the
mentions_not_resolved
branch
from
9fa23c9
to
cad3215
Compare
|
update: just merged one small PR into this PR, can you help check if the typing experience becomes better? cc @qoqobolo thank you |
@qfrank Just checked the latest build 2a1d0d9 and can say that the typing experience definitely got better, thank you. But there are two new issues: ISSUE 4: No mentions suggested when typing
|
|
@Francesca-G can we get a design opinion on the necessity of this feature? |
|
Text cant stay in composer after being sent. |
|
@pedro-et I'm asking about #16330 (comment) :) |
|
Here is the context My apologies for postponing the feature, taking into amount all the bugs and regression, I'd descope it for now. |
|
@churik, thank you for bringing up the discussion with design team! |
|
issue 4-5 should be resolved also the chance of cursor jump should be fair low now, but let's stop here ATM and maybe get back to it and continue in some time in the future :) |
@vkjr, @churik - if the issue is to what you described just now, that we want to be able to handle multiple stylings - perhaps a way to go about this is to instead just work on this component in isolation as it is in the design system. Currently almost none of the design system components related to messaging are in our quo2 component library (I believe there was some initial confusion about how to make these components separate to our state management system and that's why it was not done this way - this should be easy to do ). |
| (def mention | ||
| {:color (colors/theme-colors colors/primary-50 | ||
| colors/primary-60) | ||
| :background-color colors/primary-50-opa-10 |
There was a problem hiding this comment.
I think this color might be wrong - it should be based on the chat color or the users custom color?
Sorry @churik , only just seeing this comment now. If it makes development easier I think it's fine if we don't support partial deletions of @ mentions. What I mean by this is that once an @ mention (e.g. "@John_") has been entered/finalised in the composer, if the caret is positioned immediately to the right of the finalised @ mention and the user presses backspace then the whole @ mention should be deleted (not just the last letter in the @ mention). In terms of selection behaviour, if it helps development I think it's also fine if finalised @ mentions are treated for the purposes of selection as a single block e.g. the whole of the @ mention counts as a single letter and it's not possible to select some but not all of letters in the @ mention. Making these two changes would make it impossible to delete just part of a finalized @ mention. Do the above only if it helps with development. I may have misunderstood something about the problem or the question being asked. If I have, please clarify. |
fixes #16097
Summary
Highlight mentions in user input
Reviewer notes
If you have any idea on how to overcome the issue described in the next section, please tell me.
I also tried to implement similar mentions in the test react-native app and the similar bug is there, but barely noticeable because the basic test app is much faster.
It is not a bug in
status-gomentions implementation, because we are getting correct structure from there.And it is not a bug in generated children Text components, they are also correct.
So I believe it is a
react-nativebug.Testing notes
There is a known issue, steps to reproduce:
Result: all typed characters get the style of the mention for a fraction of a second. If you remove some characters and start typing again - the effect disappears. On the physical iPhone 13 this is barely visible. On the Android simulator, it is very visible (but it is generally terribly slow)
Video:
Screen.Recording.2023-06-23.at.13.19.45.mov
I'm not sure how critical the issue is on physical Android.
status: ready