[#18459] - Add cursor-based input handling to token-input
#18594
Conversation
ulisesmac
requested review from
briansztamfater,
J-Son89,
OmarBasem and
a team
| @@ -105,7 +142,14 @@ | |||
| (if (> num-value current-limit-amount) | |||
| (reset! input-value (str current-limit-amount)) | |||
| (reset! input-value v)) | |||
| (reagent/flush))))] | |||
| (reagent/flush)))) | |||
There was a problem hiding this comment.
@J-Son89 Just wondering if we really need the existing reagent/flush calls?
I tried removing them and I didn't find issues, but I didn't test it deeply.
| @@ -77,6 +108,7 @@ | |||
| input-value (reagent/atom "") | |||
| current-limit (reagent/atom {:amount limit-crypto | |||
| :currency token-symbol}) | |||
| input-selection (reagent/atom {:start 0 :end 0}) | |||
There was a problem hiding this comment.
Atom to track the cursor position 👀
Jenkins BuildsClick to see older builds (16)
|
| (str "0" v) | ||
| [current v input-selection-atom] | ||
| (let [{:keys [start end]} @input-selection-atom] | ||
| (if (= start end) |
There was a problem hiding this comment.
(= start end)
I'm only handling the cases when the cursor is not selecting text, validating everything seems really exhaustive.
|
@ulisesmac have you tried setting the selection onChangeText using |
Hi @OmarBasem ! Is there any advantage if we do this? |
I think it could be a cleaner and shorter solution. I see multiple new methods like So I was suggesting to try that solution as I believe it should be more concise (unless I misunderstood something from the original problem) |
|
Thanks again for the suggestion @OmarBasem ! Well, this input isn't calling
This is what it's being done in this PR, in
Since (as I said before)
I didn't add the function I think the logic of this input has to be completely reworked, because when it was implemented it didn't consider the cursor's position for validations, new chars added, and chars erased. I see this PR as a step in that direction. |
Yeah makes sense. Thanks for the clarification! |
90% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (43)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ulisesmac
force-pushed
the
18466-token-input-cursor
branch
2 times, most recently
from
e86253e to
2b7907e
Compare
pavloburykh
force-pushed
the
18466-token-input-cursor
branch
from
2b7907e to
23c1af8
Compare
|
Hi @ulisesmac , thank you for the PR. I will create separate follow-ups for this PR when I find the expected behavior from the design team. The PR is ready to be merged |
ulisesmac
force-pushed
the
18466-token-input-cursor
branch
from
c85ef01 to
01bb0bb
Compare
fixes #18459
fixes #18466
Summary
This PR adds capabilities to delete and type using the cursor position to the token input.
Screencast.from.2024-01-22.23-04-19.webm
NOTE: This PR doesn't handle the active cursor selection typing/erasing
NOT HANDLED:
It just does nothing if we press the keyboard
Review notes
Properly handling all possibilites and validations for the active cursor selection is an exhaustive task, and it's a little hard to know what do we exactly expect for every case, it's also an error-prone task.
If the handling in this PR is not the expected, I'd suggest opening a separate issue with the interactions we have and the result we expect (probably we should align with designers). I believe it's better to merge this PR since it adds parts of the total work that needs to get done.
Do we really want to reimplement this logic? inputs already handle this behavior if we use a regular keyboard (not our custom one).
Platforms
Steps to test
status: ready