Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Annotate highlighted events with notification reason #10599

Closed
wants to merge 16 commits into from

Conversation

kerryarchibald
Copy link
Contributor

@kerryarchibald kerryarchibald commented Apr 14, 2023

Fixes element-hq/element-web#24927
With matrix-org/matrix-js-sdk#3284
image

For highlighted messages, adds a notification icon to the message action bar explaining the highlight in a tooltip.
(May be expanded on later to add a dialog with rule management/anti-abuse options)

  • Adds test util for default push rules and push rule creation

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

@kerryarchibald kerryarchibald added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 14, 2023
@kerryarchibald kerryarchibald requested review from a team and removed request for a team April 17, 2023 06:38
@kerryarchibald kerryarchibald marked this pull request as ready for review April 18, 2023 04:42
@kerryarchibald kerryarchibald requested a review from a team as a code owner April 18, 2023 04:42
@kerryarchibald kerryarchibald changed the title [WIP] Annotate highlighted events with notification reason Annotate highlighted events with notification reason Apr 18, 2023
Copy link
Contributor

@justjanne justjanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good code! I've only got some optional comments regarding code style, but those are not necessarily any "better", just different.

Comment on lines 92 to 107
if (rule.rule_id === RuleId.ContainsUserName) {
return _t("Your username was mentioned.");
}
if (rule.rule_id === RuleId.ContainsDisplayName) {
return _t("Your display name was mentioned.");
}
if (rule.rule_id === RuleId.AtRoomNotification || rule.rule_id === RuleId.IsRoomMention) {
return _t("This message mentions the room.");
}
if (rule.rule_id === RuleId.IsUserMention) {
return _t("You were explicitly mentioned.");
}
if (rule.kind === PushRuleKind.ContentSpecific) {
return _t(`Your keyword '%(keyword)s' was mentioned.`, { keyword: rule.pattern! });
}
return "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a switch-case, at least according to the code style

src/components/views/messages/MessageActionBar.tsx Outdated Show resolved Hide resolved
@daniellekirkwood
Copy link

On first impression I really like this from a user perspective but the more I played with it the more I realised that it's quite odd to have a hover icon in the action bar... All other actions there have a click thru but the bell is a hover over (which might not be a11y friendly?)

As folks often miss the "(edited)" text is a button I wonder if we can combine the two things here... So we could pick a slightly different icon, on click the Edited modal appears. The hover over of the button would still tell users why they were pinged, then clicking on it will show the edits. Alternatively maybe hovering over the "(edited)" text shows the tool tip?

Either way, I think we need some design input here: @rufuskahler could you help us out?
The problem we're trying to solve is: The user doesn't know why the message text is red. It could be red for any number of reasons. Eg: Your keyword was mentioned... Your display name was mentioned...

@rufuskahler
Copy link

This is a really nice implementation @kerryarchibald – However, I have my doubts about using the action bar for the reasons Danielle mentioned. Furthermore, I'd like to discuss other ways of solving the problem closer to the root cause, e.g with Compound, we're moving away from red highlighted text for notifications. @daniellekirkwood – This sounds like more consensus is needed between Product & Design.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display why an event caused a (highlight) notification
4 participants