-
Notifications
You must be signed in to change notification settings - Fork 985
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
chore(quo): add profile - collectibles list item component #19125
Conversation
Jenkins BuildsClick to see older builds (25)
|
9479ef7
to
7a27d05
Compare
7a27d05
to
86b70bc
Compare
@@ -11,6 +11,7 @@ | |||
[:catn | |||
[:props | |||
[:map {:closed true} | |||
[:container-style {:optional true} [:maybe :map]] |
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.
cc @mmilad75 please check 🙏
[card-view props] | ||
[image-view props])]) | ||
|
||
(def ?schema |
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.
@mmilad75 please check! 🙏
@@ -55,7 +55,7 @@ | |||
quo.components.dropdowns.network-dropdown.view | |||
quo.components.empty-state.empty-state.view | |||
quo.components.gradient.gradient-cover.view | |||
[quo.components.graph.interactive-graph.view :as interactive-graph] |
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.
unneeded/unused alias
@@ -0,0 +1,36 @@ | |||
(ns quo.foundations.gradients |
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.
needed for loading state - please see comments in the pr description about this
{:state state | ||
:descriptor descriptor | ||
:component-container-style {:padding-vertical 20 | ||
:margin-horizontal 95}} |
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 views are responsive so I set margin 95 to get the view matching figma on an iphone 13
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.
Hey @J-Son89, very tidy code 👍
[rn/view {:style {:flex-direction :row}} | ||
[rn/view {:style (assoc container-style :flex-direction :row)} |
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.
👍
" | ||
width is dynamic based on available space and has to be passed to the component. | ||
This will used in large lists, to avoid recalculating the same value it is easier to pass in the value. | ||
" |
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.
This string is not a docstring 👀
btw, do we need to explicitly pass a width
?
can't we just say something like {:flex 1}
?
The problem I see with width
is we need to first get the layout, it can be done in two ways:
-
on-layout
:
It's called on the parent, meaning the first time the parent is rendered won't show its children, until it gets the dimensions to render, so this creates a one frame empty screen. -
Using window
width
orheight
:
Actually not bad, we don't skip frames ason-layout
, but the problem is we must do the calculations before mounting this component.
Sometimes we need one of these, but is it the case for this component?
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.
Hey @ulisesmac, if you look at how this component will be used (e.g https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?type=design&node-id=36420-110729&mode=design&t=qxBy1qf56zYeAYTi-4)
you'll see that this component is mostly used in a list of this component with 2 instances of it side by side with a consistent gap dependent on the screen size. For that reason I figured it would be more efficient that we calculate that once on the screen with the list rather than have that calculation done for each item etc.
So I don't think on-layout is desirable for this component here.
Pretty sure there that flex alone would not work too as I tried this approach and ran into some difficulties keeping some variants consistent with each other. Can't remember the specific issue but I believe we need to calculate width.
Will fix the docstring too 👍
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.
Question (maybe not the right thread to ask, or I misunderstood this width
prop. Correct me if I'm wrong)
If it's just the padding on the sides (vertical) that needs to be consistent across different screens and the component is used only in the list (taking the width of available of space), I believe we can use the default flex along with the :num-columns
prop in FlatList
.
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.
gonna look into it again if I can use flex here. If I recall correctly it was mostly the "type" :card
variant but perhaps I just fix the height on the bottom part (the text details and avatar image) and then leave the upper part be square.
But I think there was more than that, it was some of the empty content just wouldn't stretch for me, will have to recheck/revisit this.
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.
Yeah, I was exactly thinking about the :num-columns
prop @smohamedjavid mentioned.
@J-Son89 the current collectible listing is using that prop, just if you want an example, but I'm not sure if it can be applied to this one.
will have to recheck/revisit this.
Thanks for considering it @J-Son89 👍
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.
addressed this and now not using a fixed width. It's all responsive 👍
{:foundations [{:name :shadows | ||
{:foundations [{:name :gradients | ||
:component gradients/view | ||
} |
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.
👀
{:key :3} | ||
{:key :4} | ||
{:key :5} | ||
]} |
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.
👀
(defn- get-dimensions | ||
[size] | ||
(case size | ||
:size-24 24 | ||
:size-20 20 | ||
nil)) |
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.
Why is the size given as a prefixed keyword :size-
?
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.
having lots of random integers used across the application is not the cleanest pattern and can easily lead to changes that can break many things unknowingly. The team came to a decision a while back to use this as a standard approach for passing size props to components. See: #17279
[quo.foundations.shadows :as shadows])) | ||
|
||
(def type-card-image-height 150) | ||
(def type-card-image-height-and-border (+ type-card-image-height 2)) |
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 this use the borders-width
defined below? (def type-card-image-height-and-border (+ type-card-image-height borders-width))
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.
will double check this!
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.
thanks, so in the end I actually removed these as not needed since I made this responsive 👍
[schema.core :as schema] | ||
[utils.i18n :as i18n])) | ||
|
||
(defn- fallback |
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 word fallback
kinda vague here. Maybe fallback-view
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.
sure, can adjust
:options [{:key :size-24} | ||
{:key :size-20}]} |
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.
What is the benefit of keywordizing the size? Can't it just be 24
and 20
so that it can be used directly in component's style above without mapping it
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.
as commented above - link to team decision etc #17279 (comment)
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.
Apart from the existing comments, It looks good to me. 👍
Nice work! 🚀
{:style (style/fallback {:type type | ||
:width width | ||
:theme theme})} | ||
[rn/view |
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.
Is it possible to remove this (wrapping) view?
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.
pretty sure not, will double check!
" | ||
width is dynamic based on available space and has to be passed to the component. | ||
This will used in large lists, to avoid recalculating the same value it is easier to pass in the value. | ||
" |
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.
Question (maybe not the right thread to ask, or I misunderstood this width
prop. Correct me if I'm wrong)
If it's just the padding on the sides (vertical) that needs to be consistent across different screens and the component is used only in the list (taking the width of available of space), I believe we can use the default flex along with the :num-columns
prop in FlatList
.
[:collectible-name {:optional true} [:maybe string?]] | ||
[:community? {:optional true} [:maybe boolean?]] | ||
[:counter {:optional true} [:maybe string?]] | ||
[:gradient-color-index {:optional true} [:maybe [:enum 1 2 3 4 5]]] |
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.
perhaps I should update these to keywords 👍 :gradient-1
etc
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.
updated to this
8df3e28
to
cb61e3f
Compare
@OmarBasem, feel free to check again. Made the updates you recommended |
@Francesca-G - could you design review this component when you get a chance please? |
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.
Thanks @Francesca-G - I thought it better to leave that to be added manually. I.e that might be use case dependent. Either way it's an easy fix and something for the devs to decide as shouldn't affect the styling in anyway |
Skipping manual QA as this is just a design system component |
@status-im/mobile-qa ca you let me know if e2e tests are okay for this pr? |
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.
Nice! 💯 Thanks for the efforts on this Jamie!
@J-Son89 sure, we will review once results are ready. @ulisesmac just to make sure we do not miss, please ping us once again here once e2e results are ready. Thank you! |
98% of end-end tests have passed
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (47)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
@J-Son89 @ulisesmac e2e results are good. Ready to merge. |
fixes: #19086
designs: https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?type=design&node-id=15671-187484&mode=design&t=oSVtmnL1J5ORFvab-4
Added variant size 20 to Avatars/Collection avatar
https://www.figma.com/file/WQZcp6S0EnzxdTL4taoKDv/Design-System-for-Mobile?type=design&node-id=28941-78785&mode=design&t=j1w0ezxDk1NIja1e-4
Added Foundations Gradients to quo and a preview screen for it.
https://www.figma.com/file/v98g9ZiaSHYUdKWrbFg9eM/Foundations?type=design&node-id=4987-110&mode=dev
I decided to use Linear gradient as we need to add support for radial gradients but this can easily be done in a follow up.
I will discuss this with designers going forward
Implementation:
Foundations -> Gradient:
Card default variant
Image Default Variant
Design Review notes:
Review quo component and foundations gradients.
using linear gradient for now instead of radial gradient
Testing notes: this just adds a Quo component, no manual qa needed.