fix: communities review #18634
Merged
fix: communities review #18634
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 (44)
|
ilmotta
reviewed
Jan 26, 2024
| :description description | ||
| :title-accessibility-label :community-title | ||
| :description-accessibility-label :community-description}]) | ||
| :title-number-of-lines 2 |
There was a problem hiding this comment.
Formatting here looks strange.
Suggestion
(defn community-header
[title logo description]
[quo/text-combinations
{:container-style {:margin-top (if logo
12
(+ scroll-page.style/picture-radius
scroll-page.style/picture-border-width
12))
:margin-bottom 12}
:avatar logo
:title title
:title-number-of-lines 2
:description description
:title-accessibility-label :community-title
:description-accessibility-label :community-description}])
src/quo/components/tags/tag.cljs
Outdated
| 24 :paragraph-2 | ||
| nil)} | ||
| resource]) | ||
| (let [image-emoji? (int? resource)] |
There was a problem hiding this comment.
This part is a bit convoluted. What about this:
(defn- emoji-comp
[size resource]
(let [width (case size
32 20
24 12
nil)]
(if (string? resource)
[rn/text {:style {:margin-right 4 :font-size width}}
resource]
[rn/image
{:source resource
:style (cond-> {:margin-right 4}
(= size 32) (assoc :width width :height 20)
(= size 24) (assoc :width width :height 12))}])))
;; Then use it:
(when (= type :emoji)
[emoji-comp size resource])Changes:
- Break apart components, this allows Reagent to completely skip processing the
emoji-compif props other thansizeandresourcechange. cond->instead ofmerge+case.react-native.core/imageinstead oflegacy.status-im.ui.components.react.
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
b17bcba
to
8a43127
Compare
ilmotta
approved these changes
Jan 29, 2024
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
2 times, most recently
from
db1514a
to
a439843
Compare
ibrkhalil
approved these changes
Jan 30, 2024
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
a439843
to
eae07d8
Compare
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
eae07d8
to
9a1934a
Compare
77% of end-end tests have passedFailed tests (9)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (37)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
|
90% of end-end tests have passedFailed tests (3)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
Hey @BalogunofAfrica, could you resolve the conflict and also specify what exactly from #17566 and Figma review fixes this PR, please? |
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
9a1934a
to
87f5dd9
Compare
Hi @qoqobolo! This PR addresses all the points raised in #17566. Just to be clear I have copied everything with figma link and marked them here:
|
Ah okay, from the comments in the issue, I understood that some of them were fixed in different PRs. |
Yeah @clauxx had worked on 2 of the items but didn't raise a PR. So I cherry picked his commits (b5b6038 and 37b4cd2) and completed the items. |
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
87f5dd9
to
3ad5365
Compare
69% of end-end tests have passedFailed tests (14)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
7% of end-end tests have passedFailed tests (13)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
|
54% of end-end tests have passedFailed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (7)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
|
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
4f2bba2
to
1896008
Compare
Hi @BalogunofAfrica ! |
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
1896008
to
8a13569
Compare
|
@mariia-skrypnyk Can this be merged, I would create a follow up issue for @Francesca-G's review. |
Hi @Francesca-G !
Are you ok that it will be merged and your review feedback will be included to the followup issue created by @BalogunofAfrica? |
Sure, works for me 👍 |
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
8a13569
to
064fe06
Compare
BalogunofAfrica
force-pushed
the
fix/communities-review
branch
from
064fe06
to
b785487
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 #17566
Platform:
Figma review: https://www.figma.com/file/Tf5nfkYvpbnNCo4rKLK7lS/Feedback-for-Mobile?type=design&node-id=6117-85237&mode=design&t=Vz0i3Zn5tJ0daoMs-0#569667689
status: ready