Add badge in spaces button (PSG-944) #7088
Conversation
alfogrillo
requested review from
a team and
pixlwave
and removed request for
a team
| spacesButton.badgeText = session.map { | ||
| "\($0.spaceService.rootSpacesNotificationCount)" | ||
| } | ||
| spacesButton.badgeBackgroundColor = session.map { | ||
| $0.spaceService.rootSpacesHaveHighlightNotification ? theme.noticeColor : theme.noticeSecondaryColor | ||
| } ?? .clear |
There was a problem hiding this comment.
Should this implementation be shared with the old one on the MasterTabBarController?
Seems like using mainSession.spaceService.notificationCounter.notificationState(forAllSpacesExcept:) might be helpful rather than the private extension on the space service.
There was a problem hiding this comment.
Mmmh, the logic looks different.
Here we show the count of all unread messages including the ones in current space.
Invites to spaces are ignored unfortunately! 😅
@Johennes do you think it makes sense to cover also invites under this PR (changing the ACs)?
Not having them covered looks like a UX bug to me.
There was a problem hiding this comment.
Here we show the count of all unread messages including the ones in current space.
My bad I hadn't noticed that 🙈
There was a problem hiding this comment.
One thought looking at f1ce202. Moving the notificationState(forAllSpacesExcept: nil) up to this method would mean you could read allCount and allHighlightCount without having to iterate through the spaces twice.
There was a problem hiding this comment.
Including space invites in the highlight count seems sensible to me as I assume you otherwise might miss them just like normal messages in another space than the one you're in?
There was a problem hiding this comment.
Oh interesting. Maybe it still makes sense to sum that up? It's still an unread message of sorts so I suppose it should count as a 1.
There was a problem hiding this comment.
Or alternatively we could only use it to drive the color of the spaces icon badge?
There was a problem hiding this comment.
If we go this way probably we would need to implement both.
If you rely just on the color you could find yourself in a situation where you don't have unread messages but just invites and the badge wouldn't show. :-/
There was a problem hiding this comment.
Side note to do with as you wish: I was referring to invites as "an invite to a DM which only show up in the All Chats screen".
There was a problem hiding this comment.
If you rely just on the color you could find yourself in a situation where you don't have unread messages but just invites and the badge wouldn't show. :-/
Could we just use the red exclamation mark on the spaces icon in that case?
Side note to do with as you wish: I was referring to invites as "an invite to a DM which only show up in the All Chats screen".
Oh, I see! That's this case then:
Well, I suppose we should be covering both. This one seems more straightforward though because the invite can simply count as a red badge of 1 in the all spaces space?
Codecov ReportBase: 11.73% // Head: 11.77% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #7088 +/- ##
===========================================
+ Coverage 11.73% 11.77% +0.03%
===========================================
Files 1620 1620
Lines 159302 159293 -9
Branches 64811 64810 -1
===========================================
+ Hits 18689 18749 +60
+ Misses 139977 139906 -71
- Partials 636 638 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@amshakal could you please review the design? |
|
Kudos, SonarCloud Quality Gate passed!
|
alfogrillo
changed the title
Description
This PR add the badges for messages in the "spaces button" in the bottom bar.
The badge is meant to show the total number of unread messages across all the root spaces.
Dependency
matrix-org/matrix-ios-sdk#1645