Mark as Unread #12254
Mark as Unread #12254
Conversation
and make clering notifications also clear the unread flag
dbkr
added
the
T-Enhancement
|
This is now ready, code-wise, just needs a new icon for the mark as unread menu item and whatever testing we want to do before merging. |
|
Icon now updated |
|
Second split out PR: #12344 |
* Update `@vector-im/compound-design-tokens` in package.json (matrix-org#12339) * Change 'type' prop on badges to 'forceDot' (matrix-org#12327) * Change 'type' prop on badges tio 'forceDot' Which, hopefully, better represents what it actually does. Tidies up some of the logic. Split out from matrix-org#12254 * Missed a file * More comments * Oops, there is no count here. * Back out the logic refactor of StatelessNotificationBadge because it was also updating the logic for mark as unread badges and rewriting the ternary to the previous logic would be quite complex. * Fix doc comment Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Clarify doc on displaying the count * Update doc for the forceDot param here too. --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * [Backport staging] Update `@vector-im/compound-design-tokens` in package.json (matrix-org#12340) (cherry picked from commit e3ba643) Co-authored-by: Florian Duros <florianduros@element.io> * Fix the image view (matrix-org#12341) * TAC: Fix hover state when expanded (matrix-org#12337) * Fix TAC hover state * Add playwright test * Update playwright snapshot after last compound style changes * v3.95.0-rc.0 * v3.95.0 * Reset matrix-js-sdk back to develop branch * TAC: Order rooms by most recent after notification level (matrix-org#12329) * Order room by thread timestamp * Fix key errors in test * Update jest snapshots * Update snapshots * Rename alpha/beta to numbers * Add playwright test * Replace forceCount prop with hideIfDot (matrix-org#12344) This replaces the `forceCount` prop on room badge components with `hideIfDot` which hopefully gives a better idea of what it does, since forceCount did not really force a count. Also remove the prop where it was just passing the default value anyway. --------- Co-authored-by: Florian Duros <florianduros@element.io> Co-authored-by: David Baker <dbkr@users.noreply.github.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: ElementRobot <releases@riot.im> Co-authored-by: Robin <robin@robin.town>
|
OK, main bits of this now split out & merged separately. The next biggest chunk would be analytics but hopefully this doesn't add a great deal of complexity here. |
| @@ -102,7 +102,7 @@ export default class NotificationBadge extends React.PureComponent<XOR<IProps, I | |||
| if (notification.isIdle && !notification.knocked) return null; | |||
| if (hideIfDot && notification.level < NotificationLevel.Notification) { | |||
| // This would just be a dot and we've been told not to show dots, so don't show it | |||
| if (!notification.hasUnreadCount) return null; | |||
There was a problem hiding this comment.
why is it ok/necessary to get rid of this condition?
There was a problem hiding this comment.
I think this probably should have gone when splitting the PR out but I mistakenly left it in. We need to check only the level now because the Mark as Unread badge doesn't have a count but we still want it displayed in this case (it's considered a badge rather than a dot).
| // * The props force us to, or | ||
| // * It's just an activity-level notification or (in theory) lower and the room isn't knocked | ||
| const badgeType = | ||
| forceDot || (level <= NotificationLevel.Activity && !knocked) |
There was a problem hiding this comment.
so previously we would show a dot if count was 0 (I think?) but now we will show a badge in this case? I think?
Is that deliberate?
There was a problem hiding this comment.
Yes: because there is no unread message that triggers a notification, the count will be zero for rooms marked as unread, but we still want to display a badge.
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
* Change 'type' prop on badges tio 'forceDot' Which, hopefully, better represents what it actually does. Tidies up some of the logic. Split out from #12254 * Missed a file * More comments * Oops, there is no count here. * Back out the logic refactor of StatelessNotificationBadge because it was also updating the logic for mark as unread badges and rewriting the ternary to the previous logic would be quite complex. * Fix doc comment Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Clarify doc on displaying the count * Update doc for the forceDot param here too. --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
|
Thanks. I believe once I saw it mark a room as unread but then fail to update the UI to put the badge on, but all my attempt to repro it subsequently have failed. I'm not really sure how to proceed here. We could go ahead and merge, filing this as a bug so we're aware of it and can chase it later if we get more data, or otherwise perhaps organise another round of testing to see if anyone else can reproduce it? |
This implements the 'mark as unread' feature, as per matrix-org/matrix-spec-proposals#2867
Needs matrix-org/matrix-analytics-events#90 (plus release of matrix-analytics-events)
Checklist
Here's what your changelog entry will look like:
✨ Features