[#18458] Token input fixes #18520
Merged
[#18458] Token input fixes #18520
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
ulisesmac
requested review from
J-Son89,
OmarBasem,
mohsen-ghafouri and
a team
ulisesmac
commented
Jan 16, 2024
| (str (.toFixed (/ num-value conversion) 2) " " (string/upper-case (or (clj->js token) "")))))) | ||
| (str (.toFixed (/ num-value conversion) 2) | ||
| " " | ||
| (string/upper-case (or (clj->js token) "")))))) |
There was a problem hiding this comment.
We need a function that takes token symbols (:eth) and return some user friendly string ("ETH"), I thought it already existed, but I couldn't find it.
These conversions are very error-prone, and they've given us some runtime crashes
There was a problem hiding this comment.
Good point @ulisesmac. I have noticed that too in status-mobile. String manipulation in Clojure doesn't follow nil punning like collection functions do. It's a common source of error. I think the error prone aspect also comes from the lack of this knowledge.
I believe it's one of the reasons we have functions like safe-trim or safe-replace, because some mobile developers got tired of these nil pointer exceptions and the need to remember to check for nils, so they decided to wrap core cljs string functions. I did this too.
But these wrapping functions in utils.string are a fragile solution and require this effort to wrap cljs functions and make the code more verbose (longer function names).
To me, all of us should just remember that almost anything can be nil in mobile, therefore, string manipulation should be considered unsafe without some-> or or.
There was a problem hiding this comment.
Yes, sounds good, but, is there a way to override the string implementation?
TBH, I see this as a ClojureScript error, they are not consistent with their implementation, see string/blank?:
They are calling gstring/makeSafe why not call it in other places too?
I think it'd be ok if we just override some cljs.core implementations to add a call to makeSafe. I know, probably not the best, but IMO they are also not doing the best.
ulisesmac
commented
Jan 16, 2024
Comment on lines
+47
to
+53
| [quo/token-input (assoc @state :value @value)] | ||
| [quo/numbered-keyboard | ||
| {:container-style {:padding-bottom (safe-area/get-top)} | ||
| :left-action :dot | ||
| :delete-key? true | ||
| :on-press set-value | ||
| :on-delete delete}]]))) |
There was a problem hiding this comment.
Added the numbered-keyboard to test this input in the preview screen.
It required to modify the preview-container, now receives a full-screen? boolean
cc: @ilmotta
Jenkins BuildsClick to see older builds (19)
|
Closed
OmarBasem
reviewed
Jan 16, 2024
J-Son89
approved these changes
Jan 16, 2024
OmarBasem
approved these changes
Jan 17, 2024
ulisesmac
force-pushed
the
token-input-fixes
branch
from
e14f18a
to
548f212
Compare
VolodLytvynenko
force-pushed
the
token-input-fixes
branch
from
548f212
to
9fcde75
Compare
|
Hey @ulisesmac thank you for PR. This issue related to flickering is fixed. One minor additional issue is found which is addressed by this PR ISSUE 1: [IOS] The asset label is initially too close to 0 valueSteps:
Actual result:
input.mp4Expected result:Device:iPhone 11 Pro max, IOS 16 |
85% of end-end tests have passedFailed tests (4)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (41)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
|
Hey @VolodLytvynenko |
|
@ulisesmac, could you please extend the PR description and describe what approach you used to reduce flickering? I think that could be beneficial for the future. For example when someone will be solving similar problem and looking for related PRs) |
ulisesmac
force-pushed
the
token-input-fixes
branch
from
9fcde75
to
5561634
Compare
|
@VolodLytvynenko Here's the iOS version working: Screen.Recording.2024-01-19.at.9.55.57.p.m.movAnd Android: Screencast.from.2024-01-19.22-00-07.webm |
@vkjr The reason the input was flickering wasn't due to a state update or state propagation issue (like reagent re-renders). (cc: @Rende11 you may remember this). The issue was that the input's width was growing as the user typed, it seems this is something we should always avoid and instead set a fixed input width. But we can't set a fixed width because that token name ("ETH", "RARE", etc) should be right after the amount, and a fixed width will move the token name to the end. (check Omar's attempt, he showed some pictures). The first solution I tried was making that token name track the input's layout dimensions, The problem with this approach is that we have a very noticeable 1 frame delay, and lead to problems to render the first frame when there's no input dimensions. So I solved it by creating a "phantom" copy of the input on top of the actual input, opacity zero, no pointer events and not being able to be edited. It makes the exact spacing for "RARE"/"ETH"/... to be correctly placed. In the code the component is the function called |
ulisesmac
force-pushed
the
token-input-fixes
branch
from
7ad8062
to
2d52f9d
Compare
65% of end-end tests have passedFailed tests (14)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
|
|
hi @ulisesmac thank you for PR. No issue from my side. PR ready to be merged |
46% of end-end tests have passedFailed tests (22)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (22)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
ulisesmac
force-pushed
the
token-input-fixes
branch
from
2d52f9d
to
9045ef2
Compare
ulisesmac
force-pushed
the
token-input-fixes
branch
from
9045ef2
to
090d880
Compare
ulisesmac
added a commit
that referenced
this pull request
Jan 24, 2024
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 #18458
Summary
This PR solves the flickering we were having in the token input component:
before (taken by @VolodLytvynenko):
jump.mp4
now:
Screencast.from.2024-01-16.00-12-59.webm
Note: I'm taking multiple issues to improve the token input component, I was thinking about solving all of them in a single PR, but it's a lot of work/refactoring, I decided that it was better to open smaller PRs.
Review notes
I also modified the preview screen, so that our custom keyboard is used to test this input.
Platforms
Steps to test
Important this issue is only addressing the flickering, not the rest of the issues, they will be addressed separately.
Also notice that
is not caused or addressed by this PR.
status: ready