fix: password input triggers re-render of whole login view #18728
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 (33)
|
siddarthkay
approved these changes
Feb 6, 2024
|
This only affects dev experience. Added skip manual qa tag. |
40% of end-end tests have passedFailed tests (28)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (19)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
flexsurfer
previously requested changes
Feb 6, 2024
this change affects user first, not sure how its related to dev experience |
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
7270137 to
27d1f63
Compare
|
Fixed the re-render by moving the password input out. @flexsurfer
There should be that many re-renders because it's the input that causes the whole view re-render and debounce the input did reduce re-renders
User generally doesn't input password that fast, I use shortcuts to fill password and the password usually wrong without debounce. |
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
27d1f63 to
cc891bc
Compare
yqrashawn
changed the title
|
|
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
2 times, most recently
from
a198e3a to
4547adb
Compare
|
@yqrashawn please take a look at the issue ISSUE 1 Login button remains inactive after entering password valueSteps:
Actual result: Login button remains inactive. Unable to login the account. Expected result: login button should become active after entering >= 6 symbols in password field. telegram-cloud-document-2-5336873924753898950.mp4@yqrashawn @flexsurfer could you please explain what does this PR improves from user perspective? It is not clear for me from description what is currently wrong in terms of password input performance in our develop. |
J-Son89
reviewed
Feb 6, 2024
| (rf/dispatch [:set-in [:profile/login :password] | ||
| (security/mask-data entered-password)]) | ||
| (rf/dispatch [:set-in [:profile/login :error] ""])) | ||
| (debounce/debounce-and-dispatch [:set-in [:profile/login :password] |
There was a problem hiding this comment.
unrelated, should we not move the debounce and dispatch to the re-frame utils? I can't see how they're separate 🤷♂️
J-Son89
approved these changes
Feb 6, 2024
So the videos in description both type the input "Abcd1234::" very quickly. This only happens when you input password really quickly. |
Yes, thanks for the explanation. @siddarthkay |
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
4547adb to
cc74b9f
Compare
|
Hi @pavloburykh, ISSUE1 should be fixed now. The password in #18728 (comment) is set to empty, so only new accounts will work. |
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
cc74b9f to
aed958d
Compare
pavloburykh
force-pushed
the
fix/debounce-pwd-input
branch
from
aed958d to
5430747
Compare
pavloburykh
force-pushed
the
fix/debounce-pwd-input
branch
from
5430747 to
cfdf3bf
Compare
Hi @yqrashawn ! Not sure I understand the current update regarding password fix. |
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
cfdf3bf to
42f8ec0
Compare
|
@mariia-skrypnyk ISSUE1 is fixed. I've rebased the PR
No, cause it's fixed. The wrong password in the before video can be caught before by quick password typing before this fix.
Yes |
94% of end-end tests have passedFailed tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
|
@yqrashawn Thanks for your fix! |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
42f8ec0 to
e4f3553
Compare
Signed-off-by: yqrashawn <namy.19@gmail.com>
yqrashawn
force-pushed
the
fix/debounce-pwd-input
branch
from
e4f3553 to
e99c9c0
Compare
ibrkhalil
pushed a commit
that referenced
this pull request
Feb 25, 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 #18011
Summary
debounce pwd input for better performance
Add a 100ms denounce for password input. Only affects dev experience.
before
CleanShot.2024-02-06.at.12.28.52.mp4
after
CleanShot.2024-02-06.at.12.27.41.mp4
Testing notes
Affected area: login screen
Should have better performance
status: ready