[#21902] Reimplement community overview #22293
Merged
+1,201
−351
Conversation
This file contains hidden or 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
ilmotta,
seanstrom,
vkjr,
flexsurfer and
Parveshdhull
Jenkins BuildsClick to see older builds (41)
|
|
Please ignore the changes in ddb8c53, this was built on top of the solution for
You can get the main diff at b65d2a4 |
ulisesmac
commented
Mar 13, 2025
ulisesmac
commented
Mar 13, 2025
ulisesmac
commented
Mar 13, 2025
ulisesmac
commented
Mar 13, 2025
ulisesmac
commented
Mar 13, 2025
|
This is really good looking, a much needed improvement! I just wish there was basic file sharing ability and voice messages to make the chat features actually competitive |
flexsurfer
approved these changes
Mar 14, 2025
ilmotta
reviewed
Mar 14, 2025
There was a problem hiding this comment.
Excellent results in this PR @ulisesmac! It's a bit too large to review, but I shared what I could as feedback.
@du82 Thanks for the comment! Yes, people nowadays use audio messages too often, but due to our decentralized nature it may involve some challenges. I asked the team about it and it's something we'll evaluate, not right now but soon. |
Audio messages were available in v1.X, and so far v2.x is a downgrade. It has less functionality and feels like crypto capitalism is consuming the messaging app. The default tab used to be messaging, and now that's an afterthought. |
seanstrom
reviewed
Mar 17, 2025
Comment on lines
34
to
39
| (defn right-content | ||
| [min-size?] | ||
| (merge | ||
| {:flex-grow 1 | ||
| :flex-basis 1} | ||
| (when min-size? | ||
| {:min-height 32}))) | ||
| [min-size? centered-content?] | ||
| (cond-> {} | ||
| centered-content? (assoc :flex-grow 1 :flex-basis 1) | ||
| min-size? (assoc :min-height 32))) |
There was a problem hiding this comment.
Small nitpick:
maybe we can format these kind of assocs with a map-like structure?
for example:
(defn right-content
[min-size? centered-content?]
(cond-> {}
centered-content? (assoc :flex-grow 1
:flex-basis 1)
min-size? (assoc :min-height 32)))There was a problem hiding this comment.
Yes, we usually (and our lint-fix task) formats like that, in this case I deliberately wanted to keep it inline since it's a short map. Do you think this pattern is wrong?
There was a problem hiding this comment.
I don't think it's wrong, though I think it's easier to glance at map structure than an inline structure. For me, the inline structure reminds me of a sequence of things, but in this case we know we mean it's a sequence of key-values.
For example, here's another way of looking at it:
(def thing {:flex-grow 1 :flex-basis 1}
(def other {:flex-grow 1
:flex-basis 1}Even though it's a small map, I would still prefer seeing each key-value on its own line just for readability. Though it's not a big deal, since as you say it's a small map 🗺️
seanstrom
reviewed
Mar 17, 2025
There was a problem hiding this comment.
@ulisesmac thanks for the PR 🙌
I left a few comments, but my main comment would be about the new worklet file worklets/communities.js.
I'm wondering if we should extract some of these special number values and pass them as arguments into the JS functions from the ClojureScript functions. Not sure if that's possible, but it seems we can pass in a map with some animation configuration data.
For example, instead of hard-coding -42.5 we'll try to name that value and pass it in as an argument. Though, I would imagine we don't need to do this for every number, like 0, 1, and -1 are probably safe to keep in JS since those are likely generic animation configuration. But the special animation configuration could be passed as an argument(s). Thoughts?
ulisesmac
force-pushed
the
21902-communities-ui-artifacts-in-header
branch
2 times, most recently
from
2580d94 to
8e010b2
Compare
ilmotta
approved these changes
Mar 19, 2025
ulisesmac
commented
Mar 20, 2025
Comment on lines
+23
to
+26
| ;; NOTE: values compared against `scroll-amount` to trigger animations. | ||
| (def expand-header-threshold | ||
| "Dragging distance to collapse/extend the community." | ||
| 150) |
There was a problem hiding this comment.
Hey @seanstrom
I find very difficult to name all the values passed, those values are just style values needed to match designs due to animations, also I don't think giving names to those is significantly helpful.
Instead, I named the configurable props for animations and I'm passing them to the worklets. Since maybe the names aren't clear enough, I added a docstring to these so they can be configured more easily.
wdyt? is that enough?
ulisesmac
force-pushed
the
21902-communities-ui-artifacts-in-header
branch
from
effa284 to
fc51d3f
Compare
29% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestWalletCollectibles:
Passed tests (4)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@seanstrom I reformatted the map, thanks for the review 👍 |
|
@ulisesmac sorry for the delay, just read your comment about the naming of those special arguments. I think what you have is good, and I think your reasoning about the trouble with naming things is very valid. I suppose naming things might be a workaround for just wanting to write these JavaScript functions in ClojureScript haha. Though I imagine that's not currently possible since those worklets run in a separate environment then our main app. I wonder if later on we can experiment with adding another shadow-cljs build for the worklets in our app. Not sure if that would help or hurt things, but it could be a fun thing side-quest 🧙 |
seanstrom
approved these changes
Mar 26, 2025
|
Thanks @ulisesmac for your patience! |
81% of end-end tests have passedFailed tests (7)Click to expandClass TestWalletOneDevice:
Class TestFallbackMultipleDevice:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedThree:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (6)Click to expandClass TestCommunityMultipleDeviceMergedThree:
Class TestWalletCollectibles:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (55)Click to expandClass TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestFallbackMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestAndroid12:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestAndroid13:
Class TestWalletMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedThree:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ulisesmac
force-pushed
the
21902-communities-ui-artifacts-in-header
branch
from
768909c to
e172b2d
Compare
ulisesmac
force-pushed
the
21902-communities-ui-artifacts-in-header
branch
from
e172b2d to
7d02adb
Compare
Comment on lines
-367
to
+522
| (let [id (or id (quo.context/use-screen-params))] | ||
| (let [community-id (or id (rf/sub [:get-screen-params :community-overview])) | ||
| community (rf/sub [:communities/community-overview community-id]) |
There was a problem hiding this comment.
hi @ulisesmac, Thank you very much for improving the community screen.
While addressing conflicts, please ensure that you are not reverting any changes. Here, we are reintroducing :get-screen-params in the community screen.
There was a problem hiding this comment.
I'll fix it in my next PR, thanks for noticing
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 #21902
fixes #22236
Summary
This PR re implements the community overview page since it had multiple bugs (from design impl, to code and performance).
figma: https://www.figma.com/design/h9wo4GipgZURbqqr1vShFN/Communities-for-Mobile?node-id=10275-397889&m=dev
Test on Emulator:
Screencast.From.2025-03-13.12-29-43.mp4
iOS device:
video_2025-03-13_14-18-33.mp4
Android device:
video_2025-03-13_14-18-40.mp4
Testing notes
Please visit different communities on different devices to make sure everything looks ok. It took more time to make sure the UI looks fine on different devices with different configurations.
Please test all previous features are still working at least as before. I'd highly appreciate any bug report on it.
Platforms
status: ready