-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Communities - Show modal title and community context tag #18077
Conversation
Jenkins BuildsClick to see older builds (11)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2b1d5f2
to
2d3b523
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -185,7 +185,7 @@ | |||
[quo/button | |||
{:on-press | |||
(if config/community-accounts-selection-enabled? | |||
#(rf/dispatch [:open-modal :community-account-selection community]) | |||
#(rf/dispatch [:open-modal :community-account-selection {:community-id id}]) | |||
#(rf/dispatch [:open-modal :community-requests-to-join community])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pass the argument as line above?
instead of comunity
we use {:community-id id}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The community
map is huge, even containing encoded images. We should avoid passing it around if possible because the data (screen params) is going to be stored in the app db and used by a subscription and the view too. This puts more stress on re-frame and Reagent to compute if something needs to rendered. So here I changed the code to pass the community ID to the screen params so that the view layer can get the community from a subscription.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just left a comment.
Great job1
I mentioned this in the PR description @ajayesivan:
I've seen this problem somewhere, it's not right. |
Thank you @ilmotta. I missed that part 🙃 |
2d3b523
to
be6870d
Compare
71% of end-end tests have passed
Failed tests (8)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (34)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
|
be6870d
to
3038a44
Compare
3038a44
to
c4ddf07
Compare
@status-im/mobile-qa, I merged this PR because all its code runs behind a disabled feature toggle (we enable only during development). For the next couple weeks we will probably continue to rely mostly on e2e tests and our manual checks to make sure we respect the feature toggle and not cause any regression. The flows for joining communities shouldn't be affected at all. In the cases where we do touch code outside the toggle's umbrella we will definitely ask for your reviews :) |
Fixes #18025
Summary
This is a quick PR to make the account selection screen more in line with Figma, but more importantly to show the community context tag and a screen title.
The feature is behind a feature toggle , disabled in
develop
.Before
Demo
There's a known bug where the background of the context tag extends up to the available width of the parent container. This PR is not fixing that.
Screen_recording_20231205_124555.webm
Areas that may be impacted
None because the feature toggle is disabled in
develop
. e2e tests will be run as usual.Steps to test
status-im2.config/community-accounts-selection-enabled?
status: ready