-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat(desktop@communities): change kicked/banned member behavior #13706
Conversation
bc8aa50
to
17f14bf
Compare
Jenkins BuildsClick to see older builds (6)
|
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.
Just some minor UI remarks 👮♂️ :)
ui/app/AppLayouts/Communities/panels/CommunityBannedMemberCenterPanel.qml
Outdated
Show resolved
Hide resolved
09d377d
to
5d4dbd1
Compare
5d4dbd1
to
34b9645
Compare
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.
LGTM
@@ -44,6 +46,7 @@ proc initCuratedCommunityItem*( | |||
result.permissionModel = newTokenPermissionsModel() | |||
if tokenPermissionsItems.len > 0: |
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.
just curious if the worth of checking permissions for banned user
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.
better to leave it... in case of any regression this is another "gate" not to show the user community content. Also impact is unknown
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.
I didn't test the PR, only inspected the qml code.
I've left some comments, hope they are useful!
|
||
StatusIconTabButton { | ||
id: statusNavBarTabButton | ||
property alias badge: statusBadge | ||
property alias tooltip: statusTooltip | ||
property Component popupMenu | ||
property bool isMemberBanned: false |
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.
I'm thinking that StatusNavBarTabButton
is kind of generic component and I don't think we should add "logical / specific" behaviour in here.
WDYT if instead of adding this isMemberBanned
property in here you add something more generic?
- Rename the property with a generic name (
topIconVisible
orstateIconVisible
). - Expose the
asset
property for this icon so the consumer can decide different icons. - Expose
background
andborder
colors to be more flexible.
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.
changed
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.
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.
JoinCommunityView contains a huge amount of non-needed fields (components) that are not used in my component, so it is 2 different components
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.
I don't see them as different component since there's a lot of repeated code in here and also in ui/app/AppLayouts/Communities/panels/CommunityBannedMemberCenterPanel.qml
. The only difference from what we have now and the banned
design is just the code starting from line 98 in CommunityBannedMemberCenterPanel.qml
. I'll create a task for the UI team to refactor that but we all together as a team should try to avoid this kind of code duplications.
cc: @alexandraB99
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.
Created task: #13742
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.
Same comment than here: ui/app/AppLayouts/Communities/views/BannedMemberCommunityView.qml
I think we can reuse the existing components and add a new state to control the desired behaviour.
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.
IMHO, if we want to create one component, that will be configurable via states or any other parameters - I can create a separate ticket for the UI team because such a change will be a breaking change and regression must be tested. Also I don't have UI team vision how it must looks like
ui/app/AppLayouts/Communities/panels/CommunityBannedMemberCenterPanel.qml
Outdated
Show resolved
Hide resolved
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.
LGTM
What does the PR do
Closes: #13454
status-go PR: status-im/status-go#4806
Affected areas
Screenshot of functionality (including design for comparison)
Screen.Recording.2024-02-23.at.14.36.33.mov