Group details screen (3) #14494
Group details screen (3) #14494
Conversation
Jenkins BuildsClick to see older builds (76)
|
| @@ -21,6 +21,6 @@ | |||
| :align-items :center | |||
| :justify-content :center | |||
| :border-radius (/ container-size 2) | |||
| :background-color (colors/custom-color-by-theme color 50 60)} | |||
| :background-color (colors/custom-hex-color color 50 60)} | |||
There was a problem hiding this comment.
custom-color-by-theme is not working
There was a problem hiding this comment.
Please, also check quo2 preview while modifying quo2 components. This change is breaking the component
There was a problem hiding this comment.
@Parveshdhull that's because quo2 preview is using color names, while actually the group color comes as a hex value
There was a problem hiding this comment.
I think we need to give the preview component a list of hex values?
There was a problem hiding this comment.
yeah i think the problem here is that we store this value in db i guess, and it's just regular hex value, so it seems like we can't use custom-color-by-theme or if its not dynamic, like we have a set of colors, we could bind them, idk what's better , but for now the simplest solution is to change preview component with hex values and use colors/theme-alpha instead of creating new custom-hex-color function wdyt ?
There was a problem hiding this comment.
We are going to get rid of old colors and designs, So I would recommend we can check if provided color is a keyword or not. And if not fallback to :primary
(Not sure, but we probably not going to support group theme customization for MVP? cc: @pedro-et).
Also using stored value will not be helpful, because those colors might not work with the current app background colors per theme, and changing the group avatar to cover this case of hard-coded color instead of dynamic, might create an unwanted chain reaction (changes in other places)
There was a problem hiding this comment.
I do not think this is about group theme customization. The default group display photo is the group color. And I think it is always a hex value.
For now, I will provide some hex values for the Quo2 preview.
|
|
||
| (defn remove-members [chat-id] | ||
| (doseq [member-key @removed] | ||
| (rf/dispatch [:group-chats.ui/remove-member-pressed chat-id member-key true]))) |
There was a problem hiding this comment.
better to have a new event for the list, and move removed atom to app-db
There was a problem hiding this comment.
Currently, there is no status-go endpoint to remove a list of users. Opened an issue in status-go and will update it once we have that endpoint.
| :search? true}] | ||
| [rn/view {:style style/bottom-container} | ||
| [quo2/button {:style {:flex 1} | ||
| :on-press #(rf/dispatch [:bottom-sheet/hide]) | ||
| :disabled (zero? (count @added))} | ||
| :on-press #(do |
There was a problem hiding this comment.
I think this is a good example where fn is preferable to using the function literal #(). I scanned all the lines inside the function to see if it was using any parameter from the anonymous function. If you use (fn [] ...), it immediately tells devs there's no argument to worry about.
Another reason to avoid this is that we're trying to follow the Clojure Style Guide (not because it's the one correct way, but because it helps everyone stay on the same page). See the section https://guide.clojure.style/#no-multiple-forms-fn-literals
There was a problem hiding this comment.
Thanks for the explanation!
src/quo2/foundations/colors.cljs
Outdated
| 90 "E6" | ||
| "FF")) | ||
|
|
||
| (defn custom-hex-color [color light-opacity dark-opacity] |
There was a problem hiding this comment.
Why can't we just use the custom-color-by-theme function? It seems to support different opacity levels based on the theme, which seems to be what you need, or not?
There was a problem hiding this comment.
custom-color-by-theme is not working, not sure why
There was a problem hiding this comment.
lets find out why it doesn't work, cc @Parveshdhull
There was a problem hiding this comment.
Hi @OmarBasem,
As I can see custom-color-by-theme is not working for you for group avatars, https://github.com/status-im/status-mobile/pull/14494/files#r1041791605
- 50, 60 are color variants, not opacity
- You need to provide color as
:purpleinstead of hex value, then we will be able to find its appropriate variant, like :purple-50, :purple-60
There was a problem hiding this comment.
Hi @Parveshdhull, How can I do that? The value comes as hex value
There was a problem hiding this comment.
@OmarBasem, are we going to be able to get rid of this new function or do you really need it? This is the only thing holding me from approving the PR, but if you think there's no way around this, I'm fine. If nothing else works, it would be nice to have a TODO(OmarBasem) saying something like "custom-hex-color could be removed if XYZ is done".
There was a problem hiding this comment.
@ilmotta I have added a todo: "custom-color-by-theme supports only string colors, so maybe we can get rid of it."
There was a problem hiding this comment.
please use theme-alpha instead of creating new functions
|
Hey @OmarBasem , thank you for continuing to work on this feature! |
Sure @qoqobolo, thank you! |
OmarBasem
force-pushed
the
group-details-screen-3
branch
from
6391960
to
94ed5a4
Compare
OmarBasem
force-pushed
the
group-details-screen-3
branch
from
78518d2
to
2e1eda6
Compare
src/status_im2/subs/contact.cljs
Outdated
| (map #(first (groups %)) (distinct (map f coll))))) | ||
|
|
||
| (re-frame/reg-sub | ||
| :contacts/add-members-sections |
There was a problem hiding this comment.
@OmarBasem, could you add some tests to this subscription? This is considered a layer-3 sub, we should test them, especially a non-trivial like this one. I've recently merged a PR showing how to do it, it's almost as simple as testing any other pure function.
Test examples for layer-3 subs:
- https://github.com/status-im/status-mobile/blob/develop/src/status_im2/subs/communities_test.cljs
- https://github.com/status-im/status-mobile/blob/develop/src/status_im2/subs/wallet/wallet_test.cljs
- https://github.com/status-im/status-mobile/blob/develop/src/status_im2/subs/activity_center_test.cljs
There was a problem hiding this comment.
Sure @ilmotta, thank you!
There was a problem hiding this comment.
It's challenging I know, not the simplest of the tests to start, given its stateful nature. Thanks for trying, perhaps let's pair program on that new issue next week.
There was a problem hiding this comment.
Thanks @ilmotta, we can try again with it sometime next week.
OmarBasem
force-pushed
the
group-details-screen-3
branch
from
b93699a
to
4edef06
Compare
OmarBasem
requested review from
flexsurfer,
ilmotta,
ibrkhalil,
J-Son89 and
siddarthkay
and removed request for
flexsurfer,
ibrkhalil and
J-Son89
OmarBasem
force-pushed
the
group-details-screen-3
branch
from
a32251b
to
bda770b
Compare
|
@OmarBasem it seems like you reverted a few files in this PR |
This reverts commit e21b8d4.
|
@OmarBasem I've reverted the commit, could you please open a new PR and fix reverted files , thanks |
|
@flexsurfer sure! |
Fixes #14475
This PR implements adding members and removing members from a group.
Designs: https://www.figma.com/file/7KIYbhoqNGAIFonE0w9TDz/Messages-for-Mobile?node-id=2105%3A522742&t=HU04xJbqoDL81ZPZ-0