Account selection: Use bottom sheet component #18919
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
ilmotta
added
bug
community-account-selection
ilmotta
requested review from
FFFra,
yqrashawn,
Parveshdhull,
ajayesivan and
ibrkhalil
ilmotta
commented
Feb 20, 2024
| :customization-color color | ||
| :on-complete #(join-community-and-navigate-back id)}]]])) | ||
| (let [{id :community-id} (rf/sub [:get-screen-params]) | ||
| show-addresses-for-permissions (fn [] |
There was a problem hiding this comment.
FYI: This is a performance optimization pattern we can apply more often. The community ID or most IDs for that matter will never change, so we can extract the community ID to a let binding on the mount phase. Along with that, we can then extract anonymous functions that are re-created on every render that need to close over the community ID, therefore allowing Reagent to detect that the arguments passed to hiccup components are the same when it compares args for the current and new render.
ilmotta
commented
Feb 20, 2024
| (reagent/with-let [atom-data (reagent/atom data)] | ||
| [rn/view {:style style/container} | ||
| [rn/view {:style (merge (style/container label) container-style)} |
There was a problem hiding this comment.
FYI: style/container is a function, so this is currently a bug. I just made this component identical in behavior to its cousin settings.category.settings.view.
ibrkhalil
approved these changes
Feb 20, 2024
Comment on lines
68
to
69
| set-sheet-height #(reset! sheet-height (get-layout-height %)) | ||
| set-item-height #(reset! item-height (get-layout-height %))] |
There was a problem hiding this comment.
Nice, More performant to have state updates defined outside render fn.
Jenkins BuildsClick to see older builds (38)
|
J-Son89
reviewed
Feb 20, 2024
| @@ -5,81 +5,92 @@ | |||
| [react-native.gesture :as gesture] | |||
| [status-im.common.password-authentication.view :as password-authentication] | |||
| [status-im.contexts.communities.actions.accounts-selection.style :as style] | |||
| [status-im.contexts.communities.actions.addresses-for-permissions.view :as addresses-for-permissions] | |||
There was a problem hiding this comment.
btw, I'm trying to comment this as close to the ns as possible.
Is this accounts selection really communities specific??
Will there be other areas in the application where we want to select the specific accounts??
For example purchasing ens names might have some feature similar to this needed?
cc @clauxx
There was a problem hiding this comment.
@J-Son89, I think this question could be redirected to a designer or to the PO if you want to be really certain.
My current understanding and what I remember from interactions with designers is that the account selection is very specific to communities and the UI is built for this use case. So even if we have a similar account selection feature like you mentioned, they would probably redesign some important parts. For example, in the account selection screen, the elements displayed are all tied to the concept of token permissions, which is stored at the community level.
79% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Unable to test iOS PR build due to this issue: #18898 RPReplay_Final1708496317.MP4 |
Is this a newly created community? |
|
@yqrashawn created last week. |
can you share the community link here, I tried OCR but the page is broken |
ilmotta
force-pushed
the
ilmotta/fix/18619/use-bottom-sheet
branch
from
ff05192
to
033a0fb
Compare
|
Hi @ilmotta , thanks for the fixes! ISSUE 1: The
|
ISSUE 2: Bottom space/navigation bar color issue on AndroidFirst, please watch the videos, paying attention to the color change of the navigation bar, especially when opening the PR (color is changed to black when opening the video_2024-02-21_12-18-06.mp4Nightly (color is not changed): video_2024-02-21_12-22-46.mp4The thing is that thanks to changing the color of the navigation bar in PR, another issue became noticeable - that there is actually no bottom spacing under the In general, the issue of changing the color of the navigation bar throughout the app should be investigated separately, as the behavior is clearly incorrect in some places. But I have two questions/options within this PR:
I hope my explanation isn't too complicated and confusing. Feel free to ask any questions to clarify everything. |
|
@qoqobolo, I just noticed the following minor UI regression that's happening because the section "Eligible to join as X" appears and disappears while the check sent to status-go is happening, and because now we use a bottom sheet, which adjusts its height to the content. This can be fixed in a separate issue to speed up our process to merge this one because we have quite a few people touching the same files and rebasing has been painful. Would that be fine for you? jumpy.webm |
I would prefer if we can fix in a follow-up because it's already in develop. Do we have an issue for that already? I can surely get it done this week.
I'm gonna have a look and fix this problem in this PR :) Good finding!
I believe I just fixed this one after rebasing a pushing a few minutes ago. Maybe to avoid interrupting you too much, once I fix the bar color bug you could do another review pass to see if everything is fixed (except the bottom space issue). |
|
Everything sounds great, thanks @ilmotta!
We don't have it logged yet. I will create an issue and assign you.
Yes, totally. Just to make sure: you mean this "jumping" of the bottom sheet when its height is changing, right? I'll create an issue for this as well. |
ilmotta
force-pushed
the
ilmotta/fix/18619/use-bottom-sheet
branch
from
0b53bb8
to
fe89fda
Compare
8% of end-end tests have passedFailed tests (43)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (4)Click to expandClass TestCommunityOneDeviceMerged:
|
Ignore these results, we have another global issue... |
ilmotta
force-pushed
the
ilmotta/fix/18619/use-bottom-sheet
branch
from
fe89fda
to
7bcbcb7
Compare
@qoqobolo, do you think it will be feasible to test this PR manually again until tomorrow? Maybe it's prudent to skip e2e results this time? I'm asking this because someone merged a PR recently that forced me to do a manual rebase. Also because @ajayesivan already has cooked one other PR and he's just waiting for this one to be merged to avoid further code conflicts. No pressure really, just checking my own expectations. |
@ilmotta unfortunately, we cannot merge this particular PR without making sure that it does not break the tests, so we need fresh results. I'm really sorry about bad luck of this PR :(
Yes, that's what I was going to do today. And the good news that the issue with peers is already fixed, so I can both check the PR manually and rerun e2e. |
|
UPD: I can't test it even manually because Waku is broken again. |
25% of end-end tests have passedFailed tests (35)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (12)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@qoqobolo got it. I'll shut up! Let's not of course break the test suite. Thanks for the updates! |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
|
|
Thanks for waiting @ilmotta ! Unfortunately, Issue 3 is back, both for leaving the community from the community overview screen and from the communities home screen: 
The same thing also happens for the bottom sheet with a list of users who put a reaction. To reproduce this:
And the third place where I currently can see this is an image preview on long-press in the chat:
|
ilmotta
force-pushed
the
ilmotta/fix/18619/use-bottom-sheet
branch
from
7bcbcb7
to
8f43811
Compare
|
@qoqobolo, I tried yet another solution, even simpler. This one I fully tested in all cases and there's no bug. Let's see now! I noticed a bug while testing, and it exists in Notice how the selected item appears only when the bottom sheet is going to be closed. new.bug.in.develop.webm |
|
yes sorry it should be fixed soon |
No worries at all, thank you @flexsurfer |
80% of end-end tests have passedFailed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Can't believe it but it seems this PR is ready to be merged 😱😳 |
ilmotta
force-pushed
the
ilmotta/fix/18619/use-bottom-sheet
branch
from
b9590ce
to
04e1d57
Compare
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 #18617
Fixes #18619
Summary
Use bottom sheet component as designed in Figma.
Additionally
settings.category.reorder.view/view.Review notes
The challenge for this PR was to make the scrollable content work inside a bottom sheet, while supporting the designs for a fixed (always visible) header and footer. Using
:flex 1(as is today) in the bottom sheetcontentwrapper doesn't work. We need to tell the bottom sheetcontentto have a max height.Demo
As expected, the bottom sheet component auto adjusts to its content. Also, notice when the "Cancel" button is pressed, there's an animation (which on Android &
developdoesn't currently happen to me).feature-correct.webm
Platforms
Steps to test
Besides the two bugs being fixed by this PR, it's recommended to check how other bottom sheets are being displayed. I checked (on Android) usages of other bottom sheets and couldn't find any regression. Special care for iOS should be taken because I can't test in it.
status: ready