[#16577] Add missing context tags variants & fix API #16962
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
ulisesmac
requested review from
Parveshdhull,
smohamedjavid,
J-Son89 and
jo-mut
ulisesmac
force-pushed
the
16577-missing-context-tags-variants
branch
from
783cfe7
to
661ddce
Compare
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
ulisesmac
commented
Aug 9, 2023
src/status_im2/contexts/shell/jump_to/components/switcher_cards/view.cljs
Outdated
Show resolved
Hide resolved
Jenkins BuildsClick to see older builds (45)
|
J-Son89
reviewed
Aug 10, 2023
J-Son89
reviewed
Aug 10, 2023
src/status_im2/contexts/shell/jump_to/components/switcher_cards/view.cljs
Outdated
Show resolved
Hide resolved
ulisesmac
force-pushed
the
16577-missing-context-tags-variants
branch
from
db65236
to
d9e10e4
Compare
|
@smohamedjavid @J-Son89 I have pushed changes according to your comments. |
J-Son89
reviewed
Aug 14, 2023
| @@ -7,24 +7,27 @@ | |||
|
|
|||
| (def sizes | |||
| {:icon {:small 12 | |||
| 28 16 | |||
There was a problem hiding this comment.
how possible would it be to update the other uses here and fix across the app so we're not mixing semantic names with integers? 🤔
There was a problem hiding this comment.
As I wrote in this PR description:
So I implemented them in order to have this PR working and pass design review, but these components should be checked in the same way I did it with this one.
I meant, we need to refactor, add the missing sizes and fix the API of these components.
There was a problem hiding this comment.
Thanks, my bad for missing that!
J-Son89
reviewed
Aug 14, 2023
| :medium 32 | ||
| :large 48}}) | ||
|
|
||
| ;; TODO: this implementation does not support group display picture (can only display default group | ||
| ;; icon). | ||
| (defn- view-internal | ||
| [_] | ||
| (fn [{:keys [color size theme]}] | ||
| (fn [{:keys [color size theme icon-name] |
There was a problem hiding this comment.
can we change color to customization-color? or is that a follow up?
There was a problem hiding this comment.
This PR already has a lot of changes related to the context-tag and its uses, adding even more to refactor surrounding components is too mucho IMO. I just touched the other components to add the required features, a separate issue for each one is required. I will create the issues
J-Son89
reviewed
Aug 14, 2023
J-Son89
reviewed
Aug 14, 2023
J-Son89
reviewed
Aug 14, 2023
J-Son89
approved these changes
Aug 14, 2023
|
After rebasing with develop, the variants relying on the
are broken, I noticed also the issue: |
Francesca-G
reviewed
Aug 21, 2023
ulisesmac
force-pushed
the
16577-missing-context-tags-variants
branch
4 times, most recently
from
edfb125
to
cc65a01
Compare
@Francesca-G The preview screen blur was not looking nice on iOS but on Android it looks better. |
81% of end-end tests have passedFailed tests (8)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (35)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ulisesmac
force-pushed
the
16577-missing-context-tags-variants
branch
from
07e3b5f
to
a41eeb1
Compare
J-Son89
force-pushed
the
16577-missing-context-tags-variants
branch
from
a41eeb1
to
801f8fb
Compare
J-Son89
force-pushed
the
16577-missing-context-tags-variants
branch
from
801f8fb
to
e393ef7
Compare
|
@VolodLytvynenko, @ulisesmac is on pto so I have taken over this branch. looks like you are in testing with it so let me know if any issues and I will address them :) |
1 similar comment
|
@VolodLytvynenko, @ulisesmac is on pto so I have taken over this branch. looks like you are in testing with it so let me know if any issues and I will address them :) |
|
Hi @ulisesmac thank you for PR. No issues from my side. PR can be merged |
Add icon-name to group avatar Add missing context-tags variants and fix the API to match figma Updates context-tag previous uses remove `nil nil` params Rename alias Remove theme/provider alias Lint fix add map style argument to style/container Fix code style Update use of group-avatar api Add issue for multinetwork variant Add issue for wroing text style
J-Son89
force-pushed
the
16577-missing-context-tags-variants
branch
from
e393ef7
to
05602dd
Compare
andresceballosm
pushed a commit
to andresceballosm/status-mobile
that referenced
this pull request
Aug 29, 2023
Add icon-name to group avatar Add missing context-tags variants and fix the API to match figma Updates context-tag previous uses Update use of group-avatar api
7 tasks
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 #16577
Summary
This PR implements the missing variants of the context-tag variant and matches its API to figma.
Review notes
I noticed some other components are still incomplete:
preview-listlacked of 20 sizegroup-avatarlacked of 28 sizeuser-avatarlacked of 28 sizeSo I implemented them in order to have this PR working and pass design review, but these components should be checked in the same way I did it with this one.
Testing notes
The previous context-tag uses have been replaced by this new one reworked.
Platforms
Steps to test
status: ready