migrate reagent part 7 #19316
Merged
migrate reagent part 7 #19316
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 (23)
|
Parveshdhull
approved these changes
Mar 18, 2024
J-Son89
reviewed
Mar 18, 2024
| @@ -1,5 +1,5 @@ | |||
| import { useAnimatedReaction, withTiming, runOnJS } from 'react-native-reanimated'; | |||
| import { useState } from "react" | |||
There was a problem hiding this comment.
these changes are needed? seems to just be a clash of formatter
There was a problem hiding this comment.
it was done by lint-fix
There was a problem hiding this comment.
my bad, forgot i didn't have a formatter for js on save :(. It's strange the CI didn't fail bc of this.
J-Son89
reviewed
Mar 18, 2024
| on-press]}] | ||
| (let [theme (quo.theme/use-theme-value) | ||
| [pressed? set-pressed] (rn/use-state false) | ||
| on-press-in (rn/use-callback #(set-pressed true)) |
There was a problem hiding this comment.
it seems there is some questions on how much memoization we want to add to components. Particularly simple ui components like this which don't really have much need /use for this level of optimzation. As discussed in #19298 by @BalogunofAfrica.
It would be nice to figure this detail out before we merge all these refactor pr's so that we don't have to refactor the refactors :)
There was a problem hiding this comment.
all should be memoized, it's cheap
There was a problem hiding this comment.
@flexsurfer It does comes at a cost of more memory been used. I would be curious to see how this play out in a resource constrained (low RAM) Android device
There was a problem hiding this comment.
And this would also hold true for us memoizing every component by default
There was a problem hiding this comment.
@flexsurfer I also think memoizing everything is too much.
Memoizing callbacks is important when we pass these callbacks to components that will re-render but not all of our components suffer from it. IMO applying memoization (in general, not just React) is a process done after identifying bottlenecks, not a thing that must be done all the time since it's not for free.
Also @cammellos was in favour of making this rule more leninent:
#19033 (comment)
wdyt?
There was a problem hiding this comment.
reagent already memo hundreds of components by default, adding a few callbacks won't change the picture much, in react it's not done by default because comparison is expensive and not always possible or effective not because of memory
There was a problem hiding this comment.
Well, reagent memoizes (makes use of memo) because it needs to add a custom way to compare data across rerenders, otherwise, everything may be re-rendered all the time.
adding a few callbacks won't change the picture much
They aren't few callbacks, they'd be all the callbacks.
Anyway, again just wanted to share my thoughts on this topic.
J-Son89
reviewed
Mar 18, 2024
| :on-press-out on-press-out | ||
| :style (style/add-account-container {:theme theme | ||
| :metrics? metrics? | ||
| :pressed? pressed?})} |
There was a problem hiding this comment.
btw the reason this is a pressable wrapping a button is to expand the styling beyond the button, which is centered in the component.
as far as I recall the thing is that the border of this component also has some pressed state styling to it so it needs to be aware of when it's pressed in etc as well.
Perhaps there was something more to it but pretty sure it was along that lines of why this was handled this way.
There was a problem hiding this comment.
i just removed on-press handlers from the button
There was a problem hiding this comment.
yeah makes sense, I wonder does that affect the styles of the button in pressed state then.
J-Son89
approved these changes
Mar 18, 2024
31% of end-end tests have passedFailed tests (32)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (15)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
clauxx
approved these changes
Mar 20, 2024
85% of end-end tests have passedFailed tests (6)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
@flexsurfer can you please elaborate on what should be tested in this PR and what is the possible scope of regression and also resolve the conflicts and reabse PR? |
flexsurfer
force-pushed
the
feature/migrate-reagent-part7
branch
2 times, most recently
from
1f64cba
to
810674f
Compare
|
@churik added description, thank you |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
|
|
@flexsurfer thanks for the PR. Please take a look at the issue ISSUE 1 '+' icon inside Add new account button does not respond on tapSteps:
Actual result: '+' icon does not react in tap. Rest area of the Add new account button reacts on tap. telegram-cloud-document-2-5244745854052485599.mp4 |
flexsurfer
force-pushed
the
feature/migrate-reagent-part7
branch
from
1885656
to
b36f9d3
Compare
|
good catch @pavloburykh , fixed |
52% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (27)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@flexsurfer thanks for the fix. Failed e2e are not PR related. Ready for merge. |
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.
this is the latest change for quo, and it's free from reagent :)
Affected quo components:
wallet/account_card
wallet/amount_input
wallet/keypair
wallet/network_routing
wallet/token_input
wallet/wallet_activity
For QA: A smoke test should be enough, but pls contact me if you need more info or help