matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

Published 20 hours ago • verified publisher icon matrix-org

Change the text color of highlighted messages

Open robintown opened this issue 2 years ago • 9 comments

This is a reintroduction of the changes from https://github.com/matrix-org/matrix-react-sdk/pull/5724, since @niquewoodhouse expressed interest in doing a design review, but the author was not available to update the branch.

. Before After
Modern layout Screenshot 2022-08-17 at 08-18-51 Element #element-dev matrix org Screenshot 2022-08-17 at 08-18-33 Element #element-dev matrix org
Bubble layout Screenshot 2022-08-17 at 08-19-08 Element #element-dev matrix org Screenshot 2022-08-17 at 08-17-50 Element #element-dev matrix org

Closes https://github.com/vector-im/element-meta/issues/744

Checklist

  • [x] Linter and other CI checks pass

Here's what your changelog entry will look like:

✨ Features

  • Change the text color of highlighted messages (#9199). Fixes vector-im/element-meta#744.

robintown avatar Aug 17 '22 12:08 robintown

Thanks for taking this over, I haven't had time to do much side work since I started a full time job. ❤️

nhhollander avatar Aug 17 '22 15:08 nhhollander

@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

nadonomy avatar Oct 13 '22 08:10 nadonomy

@nadonomy I'm going to attempt to add keyword pills to this PR, hopefully later this week when I have more time

robintown avatar Oct 17 '22 22:10 robintown

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 :(

ara4n avatar Oct 17 '22 23:10 ara4n

(removing the highlight colour entirely, as per the current PR, seems like a major step backwards?)

ara4n avatar Oct 17 '22 23:10 ara4n

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.

Avamander avatar Feb 14 '23 15:02 Avamander

@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 avatar Mar 30 '23 17:03 kenwuuu

Screen Shot 2023-03-31 at 16 05 35

Looks like replies to replies to your messages have the desired appearance that you guys are looking for? Should help you track down the issue and code a fix.

kenwuuu avatar Mar 31 '23 21:03 kenwuuu

@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:

Screenshot 2023-04-05 at 15-03-35 Element 7 Matrix HQ

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.

robintown avatar Apr 05 '23 19:04 robintown