Share Screen -> Share Wallet Address -> Indicators #18523
Merged
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
Pau1fitz
commented
Jan 16, 2024
Comment on lines
101
to
103
| {:should-component-update | ||
| (fn [_ [_ old-item] [_ new-item]] | ||
| (not= old-item new-item)) |
There was a problem hiding this comment.
this is required to prevent wallet-qr-code-item component being re-rendered in the flatlist when the value of the current-index atom was updated.
Jenkins BuildsClick to see older builds (111)
|
Parveshdhull
requested review from
smohamedjavid,
J-Son89,
OmarBasem and
mmilad75
OmarBasem
reviewed
Jan 22, 2024
Comment on lines
97
to
103
| (defn- render-item | ||
| [_] | ||
| (let [width (rf/sub [:dimensions/window-width])] | ||
| (reagent/create-class | ||
| {:should-component-update | ||
| (fn [_ [_ old-item] [_ new-item]] | ||
| (not= old-item new-item)) |
There was a problem hiding this comment.
Please use a functional component
There was a problem hiding this comment.
actually I think you just need to use memo - https://legacy.reactjs.org/docs/hooks-faq.html#how-do-i-implement-shouldcomponentupdate
There was a problem hiding this comment.
updated to use Memo
J-Son89
reviewed
Jan 22, 2024
J-Son89
reviewed
Jan 22, 2024
J-Son89
reviewed
Jan 22, 2024
J-Son89
approved these changes
Jan 23, 2024
briansztamfater
approved these changes
Jan 23, 2024
mkae lint fix remove unneccessary props
58% of end-end tests have passedFailed tests (17)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (28)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
|
88% of end-end tests have passedFailed tests (3)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
Hi @Pau1fitz ! Nice to meet you! I've checked the case when user has only one wallet and indicator is present, here is how it looks like : I went to designers and they said it is not needed in case "one wallet - one indicator": It would be cool if you can handle this at this PR. |
|
hey @mariia-skrypnyk 👋 PR updated to hide to indicators when only one item - see below. 
|
Hi @Pau1fitz ! IMG_6462.MP4 |
|
@mariia-skrypnyk the code has now been updated and fixes this blink when switching to wallet view |
mariia-skrypnyk
added
test cases required
test cases added
and removed
test cases required
labels
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.
Related to #18452
Share Screen -> Share Wallet Address -> Indicators
Screen.Recording.2024-01-16.at.08.24.12.mov
iOS
Android
Testing notes:
Navigate to the share screen
click on Wallet
user can see indicators for number of wallets and current wallet being viewed