Update "Preview List" component to use "Number Tag" for overflow item #17257
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 (28)
|
J-Son89
approved these changes
Sep 12, 2023
smohamedjavid
force-pushed
the
feature/preview-list-follow-up
branch
from
8f93cf4
to
5362297
Compare
77% of end-end tests have passedFailed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
smohamedjavid
force-pushed
the
feature/preview-list-follow-up
branch
from
5362297
to
88063f0
Compare
ulisesmac
approved these changes
Sep 14, 2023
Comment on lines
-82
to
+34
| {:source (:source item) | ||
| {:source (or (:source item) item) | ||
| :style {:width size | ||
| :height size | ||
| :border-radius border-radius}}] | ||
|
|
||
| (:tokens :network :dapps) [fast-image/fast-image | ||
| {:source (:source item) | ||
| {:source (or (:source item) item) |
There was a problem hiding this comment.
I guess item can be a map and at the same time it can be a value (i guess a string). but this is not safe to do, I'd suggest to just be one type of thing.
The problem is we could get an exception while trying to extract a key from a string if we attempt: (:source "hey")
There was a problem hiding this comment.
Yes, @ulisesmac. The item can be a map or number (for local resources) or string (for URL).
The problem is we could get an exception while trying to extract a key from a string if we attempt: (:source "hey")
We won't get any exceptions. The form (:source "hey") will produce nil. And since we use the or method, it will fall back to item.
There was a problem hiding this comment.
Oh, ok, it seems Clojurescript is pretty flexible sometimes.
Thanks!
Comment on lines
101
to
105
| [preview-list/view {:type :user :size :size/s-20} | ||
| users] | ||
|
|
||
| :multinetwork | ||
| [preview-list/view {:type :network :size 20} | ||
| (map #(hash-map :profile-picture %) networks)] | ||
| [preview-list/view {:type :network :size :size/s-20} |
There was a problem hiding this comment.
Nice 👍 !!!
But still I don't agree to adding that s- preffix because how will we know when it's an s, m or l? I know this is not related to this PR, but wanted to mention it
There was a problem hiding this comment.
the s- is confusing people so let's remove it. Just :size-20 etc is fine 👍 will be putting this in the docs
There was a problem hiding this comment.
I will update it in this PR for the preview-list and number-tag components and their usage across the codebase. 👍
Comment on lines
57
to
65
| (defn container | ||
| [{:keys [type number size blur? theme]}] | ||
| {:style {:width (get-width size number) | ||
| :height (get-in sizes [size :size]) | ||
| :border-radius (get-shape-value size :border-radius type) | ||
| :justify-content :center | ||
| :align-items :center | ||
| :background-color (get-bg-color blur? theme)}}) | ||
| [{:keys [type number size blur? theme container-style]}] | ||
| {:style (merge {:width (get-width size number) | ||
| :height (get-in sizes [size :size]) | ||
| :border-radius (get-shape-value size :border-radius type) | ||
| :justify-content :center | ||
| :align-items :center | ||
| :background-color (get-bg-color blur? theme)} | ||
| container-style)}) |
There was a problem hiding this comment.
container-style could override the width and height of the component, and maybe we don't want it.
So, wdyt about:
(assoc container-style :width ... :height ... :border-radius ... )` to preserve the original styles even if they try to override them?
| :number 4 | ||
| :more-than-99-label "99+"}) | ||
| (let [state (reagent/atom {:type :accounts | ||
| :size :size/s-32 |
There was a problem hiding this comment.
again, about s-32, why not m-32 or l-32? do we have a rule to know when to use each letter?
There was a problem hiding this comment.
let's use :size-32 - see discussion and result here:
#17279
btw the idea was to always use -s as in -size, so clearly that idea was confusing 😆
smohamedjavid
force-pushed
the
feature/preview-list-follow-up
branch
from
88063f0
to
44e3265
Compare
churik
force-pushed
the
feature/preview-list-follow-up
branch
from
a9a174f
to
ae8aca6
Compare
77% of end-end tests have passedFailed tests (10)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
smohamedjavid
force-pushed
the
feature/preview-list-follow-up
branch
from
ae8aca6
to
c89af9c
Compare
86% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Thanks @smohamedjavid, LGTM. |
|
Thanks for testing the PR @qoqobolo. Since @Francesca-G is away. I believe this PR needs to wait. |
|
@smohamedjavid yeah, Pedro said that they were gonna find someone for this week for PRs review (Discord message) |
Francesca-G
reviewed
Sep 25, 2023
There was a problem hiding this comment.
- Preview lists: the component looks good ✨
- Context tags: here's the review
smohamedjavid
force-pushed
the
feature/preview-list-follow-up
branch
2 times, most recently
from
a21d91f
to
bada48c
Compare
|
@Francesca-G Thank you for reviewing this PR! I have addressed the comments from Design feedback for the Context Tag component. Please review. |
Francesca-G
approved these changes
Sep 25, 2023
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/preview-list-follow-up
branch
from
bada48c
to
6c59263
Compare
ibrkhalil
pushed a commit
that referenced
this pull request
Oct 1, 2023
…#17257) This commit updates - the preview-list component to use "number-tag" for overflow items in the list as a follow-up of the PR Update "Preview list" component. - the usage of the preview-list component in the context-tag (multiuser and multinetwork type) is updated as it was broken. - the size APIs of preview-list and number-tag and their usage across the codebase to respect the new guidelines. --------- Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
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 #17256
Summary
This PR updates
the
preview-listcomponent to usenumber-tagfor overflow items in the list as a follow-up of PR Update "Preview list" component #17051 (review).the usage of the
preview-listcomponent in thecontext-tag(multiuserandmultinetworktype) is updated as it was broken.the size APIs of
preview-listandnumber-tagand their usage across the codebase to respect the new guidelines.Testing notes
It's good to have this PR go through QA as these components are used in several screens and through quick design review.
Platforms
Steps to test (design review)
Preview List component
Quo2 preview > list-items > preview-listsContext Tag component
Quo2 preview > tags > context-tagsstatus: ready