This repository was archived by the owner on Sep 11, 2024. It is now read-only.
Show the number of unread notifications above the bell on the right#2336
Merged
Conversation
Fixes element-hq/element-web#3383 This achieves the result by counting up the number of highlights across all rooms and setting that as the badge above the icon. If there are no highlights, nothing is displayed. The red highlight on the bell is done by abusing how the Tinter works: because it has access to the properties of the SVG that we'd need to override it, we give it a collection of colors it should use instead of the theme/tint it is trying to apply. This results in the Tinter using our warning color instead of whatever it was going to apply. The RightPanel now listens for events to update the count too, otherwise when the user receives a ping they'd have to switch rooms to see the change.
Member
Author
|
This has a bug where when reading (or receiving new) notifications the icon doesn't switch colors and remains stuck. |
… source The js-sdk's placement of the notification change was unreliable and could cause stuck notifications. The new location (piggybacking the Notifier) is a lot more reliable. The tinting has been changed fairly invasively in order to support the changing of the `fill` attribute. What was happening before was the `fill` property would happily get set to the forced color value, but when it came time to reset it it wouldn't be part of the colors array and fail the check, therefore never being changed back. By using a second field we can ensure we are checking the not-forced value where possible, falling back to the potentially forced value if needed. In addition to fixing which color the Tinter was checking against, something noticed during development is that `this.colors` might not always be a set of hex color codes. This is problematic when the attribute we're looking to replace is a rgb color code but we're only looking at `keyHex` - the value won't be reset. It appears as though this happens when people use custom tinting in places as `this.colors` often gets set to the rgb values throughout the file. To fix it, we just check against `keyHex` and `keyRgb`.
Member
Author
|
Alright, I've fixed this and removed the dependency on the js-sdk for the change. More information about what went wrong is in 95d15b7. |
dbkr
reviewed
Dec 7, 2018
| if (payload.action === "view_user") { | ||
| if (payload.action === "event_notification") { | ||
| // Try and re-caclulate any badge counts we might have | ||
| this.forceUpdate(); |
Member
There was a problem hiding this comment.
Will you not need an equivalent for when the user reads the notification and the count goes down again?
Member
Author
There was a problem hiding this comment.
You would think so, but something somewhere magically makes it work. I tested in Chrome and Firefox and for some reason it happily goes down but doesn't magically go up.
dbkr
approved these changes
Dec 13, 2018
This was referenced Dec 13, 2018
dbkr
added a commit
that referenced
this pull request
Jan 3, 2019
Does not include #2336 as the file has been moved out from underneath it: will do this separately
This was referenced Jan 3, 2019
turt2live
added a commit
that referenced
this pull request
Jan 3, 2019
Part 2 of 3: Merge develop->experimental minus #2336
This was referenced Sep 4, 2022
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Requires matrix-org/matrix-js-sdk#802Fixes element-hq/element-web#3383
This achieves the result by counting up the number of highlights across all rooms and setting that as the badge above the icon. If there are no highlights, nothing is displayed. The red highlight on the bell is done by abusing how the Tinter works: because it has access to the properties of the SVG that we'd need to override it, we give it a collection of colors it should use instead of the theme/tint it is trying to apply. This results in the Tinter using our warning color instead of whatever it was going to apply.
The RightPanel now listens for events to update the count too, otherwise when the user receives a ping they'd have to switch rooms to see the change.
Darker color is me testing tinting.