[Quo2] Implement Account Avatar Component
#16795
Conversation
Jenkins BuildsClick to see older builds (36)
|
| @@ -14,7 +14,7 @@ | |||
| (defn avatar | |||
| [avatar-props] | |||
| [rn/view {:style (style/avatar-border)} | |||
| [account-avatar/account-avatar (assoc avatar-props :size 48)]]) | |||
| [account-avatar/view (assoc avatar-props :size 48)]]) | |||
There was a problem hiding this comment.
The Account Avatar component is used in this component settings > accounts. I checked for usage across the codebase, but nothing was found.
It was introduced in this PR: #14643. I checked for the designs attached to this PR, looks like it was removed.
smohamedjavid
requested review from
erikseppanen,
briansztamfater,
J-Son89,
OmarBasem and
ulisesmac
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
671a100
to
ab43986
Compare
|
it is also used in src/quo2/components/tabs/account_selector.cljs - well it's supposed to be anyway if you look at the designs - perhaps you can update that here too? |
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
ea405ad
to
588dc3b
Compare
src/status_im2/contexts/quo_preview/avatars/account_avatar.cljs
Outdated
Show resolved
Hide resolved
| (defn get-border-radius | ||
| [size] | ||
| (case size | ||
| 80 16 | ||
| 48 12 | ||
| 32 10 | ||
| 28 8 | ||
| 24 8 | ||
| 20 6 | ||
| 16 4)) | ||
|
|
||
| (defn get-emoji-size | ||
| [size] | ||
| (case size | ||
| 80 36 | ||
| 48 24 | ||
| 32 15 | ||
| 28 12 | ||
| 24 11 | ||
| 20 11 | ||
| 16 11)) |
There was a problem hiding this comment.
This size param is passed directly from the component props, it means if we pass an invalid size while calling the component this case statement will crash with an exception. So please add the default value.
Also, I find a little hard passing a number for size, I mean, we have a set of valid sizes and any other number will fail, so maybe we could create a set of keywords (small, medium, or sm, md, etc), but I know this doesn't match with figma, so is hard.
Wdyt?
There was a problem hiding this comment.
I agree with the sentiment @ulisesmac. The problem is that the DS is built with non-semantic spaces and sizes. If we can convince them to change how they manage such a foundational aspect of their work then it would help create a more maintainable DS, but good luck convincing them about this 🤷🏼 It was always shocking to me that the entire app is designed without a grid system in mind. Many sizes/spaces are chosen arbitrarily (their own words), although they do try to follow some patterns, like multiples of 4.
As you hinted, we could come up with a standardized list of sizes and each component could use those instead of numbers, like what's used in Tailwind https://tailwindcss.com/docs/responsive-design. I actually like this and have commented a few times with @J-Son89. But this can also cause some confusion and extra mental effort to translate them to what's used in Figma and can also increase the likelihood of bugs. Since now we sort of want to just mimic what's in Figma to avoid these erroneous code<->Figma translations, the numbers might be okay for now.
On the other hand, it's a serious technical mistake to build the mobile app without responsiveness in mind from the get go. And responsiveness requires us to not hardcode magical numbers. It'll be very expensive (development-wise) to do this in the future. Flip phones and foldable phones are a thing now, and obviously tablets. Screen sizes are not that fixed little world of iPhone 11 used by the DS. It's just 😢. I know of many users who love their tablets and would like to use a "super app" such as Status in their bigger screens. Not to count many users like to read with their phones on the horizontal. Anyway, responsiveness and semantic sizes seem to be distant in the future...
This size param is passed directly from the component props, it means if we pass an invalid size while calling the component this case statement will crash with an exception. So please add the default value.
I really miss clj-kondo not having a rule for forcing a default result in case. I'm considering creating a new linter rule for this. I really don't see any benefit in risking throwing exceptions, so to me the rule is simple, we should always add a default value in case.
There was a problem hiding this comment.
Also, I find a little hard passing a number for size, I mean, we have a set of valid sizes and any other number will fail, so maybe we could create a set of keywords (small, medium, or sm, md, etc), but I know this doesn't match with figma, so is hard.
I understand @ulisesmac, It's a bit painful, I'm not a fan of numbers too as you mentioned it's error-prone, but we have to maintain the synchronisation between Figma and Quo2 implementation. I added a default value.
On the other hand, it's a serious technical mistake to build the mobile app without responsiveness in mind from the get go. And responsiveness requires us to not hardcode magical numbers. It'll be very expensive (development-wise) to do this in the future. Flip phones and foldable phones are a thing now, and obviously tablets. Screen sizes are not that fixed little world of iPhone 11 used by the DS. It's just 😢. I know of many users who love their tablets and would like to use a "super app" such as Status in their bigger screens. Not to count many users like to read with their phones on the horizontal. Anyway, responsiveness and semantic sizes seem to be distant in the future...
I don't encourage using pixel value as it's not responsive and will have different effects based on the DPI/PPI of the user's device. As you mentioned, it will become more tedious to refactor if we decide to be responsive in future. Can't argue 😢. This is more of a team discussion.
There was a problem hiding this comment.
I don't encourage using pixel value as it's not responsive and will have different effects based on the DPI/PPI of the user's device. As you mentioned, it will become more tedious to refactor if we decide to be responsive in future. Can't argue cry. This is more of a team discussion.
For sure @smohamedjavid, this is a team-level discussion. I brought up to see how you guys reacted to it and if we're more on less on the same page. Nothing that blocks your PR.
| {:label "Emoji" | ||
| :key :emoji | ||
| :type :text} | ||
| {:label "Customization color:" |
There was a problem hiding this comment.
FYI @smohamedjavid, I recently added a function to help us be more DRY. https://github.com/mohsen-ghafouri/status-react/blob/43cba161d66255ec8983f08d74e6050643dd0b34/src/status_im2/contexts/quo_preview/preview.cljs#L226
But I think the better implementation might be to use this color-picker/color-list you used in this PR and which I've seen other PRs used as well.
There was a problem hiding this comment.
Thanks, @ilmotta! This helps a lot.
I'm not sure about using color-picker/colour-list as we have :no-color option, which we agreed to remove due to some reason/issues from the Desktop app.
IMO, even the colour-list declaration (and usage in preview screens) in the color-picker component should get the colours from quo2.foundations.colors ns as it is our source of truth.
There was a problem hiding this comment.
Perhaps we can update the other uses in the preview screens to whatever you decide upon too.
I agree with your point @smohamedjavid but no strong opinions on this 👍
There was a problem hiding this comment.
It's not clear to me either, I would just welcome such generalization because I keep seeing PRs repeating this field in preview screens. I'll keep an eye in my next PRs for quo components, and if this is not improved until then I'll try to do something, should be quick.
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
2 times, most recently
from
ac06d1e
to
8d1b460
Compare
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
8d1b460
to
6576d3d
Compare
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
6576d3d
to
884ca0b
Compare
85% of end-end tests have passedFailed tests (6)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (34)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@Francesca-G - Can we get a design review on this component, please? |
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
884ca0b
to
c4c3423
Compare
|
@Francesca-G - Regarding issue 1, We use the same size (px) for height and width as mentioned in Figma. 
We don't use any value less than mentioned in Figma. For issue 2, Please check as I updated the emoji to fit in the container for small-size variants. |
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
smohamedjavid
force-pushed
the
feature/account-avatar-changes
branch
from
1cde56c
to
b867363
Compare
fixes #16761
Summary
This PR implements the
Account Avatarcomponent which is needed for wallet screen development.Design
Link to Figma
Review notes
Since this wallet account accepts only emoji as a unique indicator. We use the
rn/textcomponent to display emoji.Platforms
Steps to test
Quo2 Preview > avatar > account avatarstatus: ready