Community token gating component #19642
Merged
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
ajayesivan
changed the title
Jenkins BuildsClick to see older builds (58)
|
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
2 times, most recently
from
4700b9c
to
83b9e97
Compare
ajayesivan
requested review from
ilmotta,
flexsurfer,
Parveshdhull and
BalogunofAfrica
ilmotta
approved these changes
Apr 19, 2024
| {:container-style style/divider} | ||
| [text/text | ||
| {:size :label | ||
| :style {:color (colors/theme-colors colors/neutral-50 colors/neutral-40)}} |
There was a problem hiding this comment.
It's better if we can pass the theme argument from the parent component view to theme-colors. Eventually we will be able to completely remove the dependency of theme-colors in the global state quo.theme/theme-state.
| :style {:color (colors/theme-colors colors/neutral-50 colors/neutral-40)}} | ||
| (string/lower-case (i18n/label :t/or))]])]) | ||
|
|
||
| (defn view-internal |
There was a problem hiding this comment.
Quick one: missing private metadata.
| :style style/you-hodl} | ||
| (i18n/label (if satisfied? :t/you-hodl :t/you-must-hold))] | ||
| (map-indexed (fn [index tokens-item] | ||
| ^{:key (str role "-tokens-" index)} |
There was a problem hiding this comment.
It's unnecessary to concatenate strings for humans to form the unique key.
| [tokens-row | ||
| {:tokens tokens-item | ||
| :first? (zero? index) | ||
| :divider? (= index (dec (count tokens)))}]) |
There was a problem hiding this comment.
We can extract this calculation from the loop, and thus calculate the last index once outside.
Also, if I'm not my mistaken, count is O(N) on sequences, so not ideal to be called in a loop.
| (let [theme (theme/use-theme-value)] | ||
| [rn/view {:style (style/container theme)} | ||
| [rn/view | ||
| {:style style/eligibility-row} |
There was a problem hiding this comment.
If possible while reviewing your code for style, consider checking if you can join these {:style ...} lines on the line preceding it. There are lots of these unnecessary line breaks in the codebase (it's not just from your PR), and unfortunately for us, zprint can't auto-format this case without causing other headaches. A few lines below the {:on-press ...} can also be moved to the previous line.
| @@ -140,46 +121,39 @@ | |||
| highest-permission-role]} (rf/sub [:community/token-gated-overview id]) | |||
| highest-role-text | |||
| (i18n/label | |||
| (communities.utils/role->translation-key highest-permission-role :t/member))] | |||
| (communities.utils/role->translation-key highest-permission-role :t/member)) | |||
| info-button-handler (fn [] | |||
There was a problem hiding this comment.
Since the function is not closing over any value, it's better to extract it to its own var for performance reasons and for clarity.
| :on-press (if config/community-accounts-selection-enabled? | ||
| #(rf/dispatch [:open-modal :community-account-selection-sheet | ||
| {:community-id id}]) | ||
| #(rf/dispatch [:open-modal :community-requests-to-join |
There was a problem hiding this comment.
We can cache these functions with use-callback, especially because they only depend on the id, which basically won't ever change. This should help Reagent not needlessly process community-token-gating.
Parveshdhull
approved these changes
Apr 22, 2024
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
2 times, most recently
from
c8d2b9e
to
2a4ed2d
Compare
87% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
|
BalogunofAfrica
approved these changes
Apr 26, 2024
|
Hi @ajayesivan ! Thanks for your PR! Does your PR affect these two elements? |
|
Hi @ajayesivan ! |
|
Hi @mariia-skrypnyk, Apologies for the delay in reply. I was AFK.
This PR implements the whole "Community token gating" component. Which means all variations should work as per the design.
Let me know if you have further questions. Thank you! |
Hi @ajayesivan ! |
|
@mariia-skrypnyk that makes sense. We can skip manual qa. |
|
@Francesca-G can you review when you have a time? Thanks!! |
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
from
2a4ed2d
to
61f4821
Compare
|
Hey @ajayesivan ! Please take a look at design review of Francesca placed above 👆. |
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
3 times, most recently
from
4da5d7c
to
9e82bb3
Compare
|
Hi @Francesca-G, I have fixed the spacing issue. The button text color issue in the disabled state is not related to this PR. It seems like an issue with the button component implementation. The fix may not be straightforward since the opacity property behaves differently in iOS and Android as mentioned here. Also, the button component has many variations and requires a good amount of testing when introducing a change to avoid further regressions. Is it okay if we address it as a follow-up? |
The vertical spacing seems to be only fixed in the top section where the text is. Please have a look here 
Sure 👍 |
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
3 times, most recently
from
986ff43
to
16109aa
Compare
|
Hi @Francesca-G, I have fixed the design issues. Could you take another look? Thanks! 
|
|
Followup issue: #20056 |
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
2 times, most recently
from
9eeaa2e
to
0a969fb
Compare
ajayesivan
force-pushed
the
19320-community-token-gated-component
branch
from
0a969fb
to
71abfe3
Compare
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 #19320
Summary
This PR implements the
community/community-token-gatingcomponent and uses it in the community overview screen.Resulting fixes or changes
token-gatingcomponentAreas that may be impacted
Screenshots
status: ready