-
-
Notifications
You must be signed in to change notification settings - Fork 827
Change the text color of highlighted messages #9199
Conversation
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.
Tested the deployed version. LGTM 👍
Thanks for taking this over, I haven't had time to do much side work since I started a full time job. ❤️ |
@niquewoodhouse bump on review/iteration on this @robintown thx for picking up this PR. How difficult is it to stylise the words or mentions causing the highlight? These designs are old, but should illustrate the desired implementation when we explored this before: https://www.figma.com/file/M15i7l4PSPY1B5M7Zhu80H/Notifications?node-id=2015%3A16302 |
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.
👍
@nadonomy I'm going to attempt to add keyword pills to this PR, hopefully later this week when I have more time |
fwiw the intention on highlights was always to only highlight the content that triggered the bing rather than the whole line - the "whole line goes red" was a quick hack that we (I?) did, and then got stuck :( |
(removing the highlight colour entirely, as per the current PR, seems like a major step backwards?) |
Could this be merged? The red color constantly confuses users and looks seriously ugly. Removing it would be a step forward until the messages are highlighted properly. |
@robintown, or anyone else: I don't understand why keyword highlights blocks this from being merged, could I get some clarification? This doesn't depend on highlights, and highlights don't depend on this. Please publish this PR, this should get this merged in because that's how we make progress, with small incremental changes. Do we need this many reviewers also? |
@kenwuuu, Here's an example that should show why we need to turn keyword highlights into pills if we are to remove the red text color: Here I've set up a keyword for "video rooms", which is why the first and second messages have notified me, but because I've clicked on a link to the second message, I can't see the yellow background color. So, there needs to be some secondary indication that this message has caused a highlight. |
This is a reintroduction of the changes from #5724, since @niquewoodhouse expressed interest in doing a design review, but the author was not available to update the branch.
Closes element-hq/element-meta#744
Checklist
Here's what your changelog entry will look like:
✨ Features