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

Changed text color of highlights #5724

Closed
wants to merge 1 commit into from
Closed

Changed text color of highlights #5724

wants to merge 1 commit into from

Conversation

nhhollander
Copy link

@nhhollander nhhollander commented Mar 6, 2021

This is a reintroduction of the changes from element-hq/element-web#2896.

Previously this change was rejected because in the absence of a pill, it would be impossible to tell if a message was highlighted or not. Since fc3c4fc a yellow background has been added to highlighted messages making the red text redundant. I believe this also resolves the concerns discussed in in element-hq/element-web#2896.

This should address some issues with element (and probably a few more I'm missing):
element-hq/element-meta#744
element-hq/element-web/issues/13963

Dark Theme:
Before:
element_before_c

After:
element_after_c

Light Theme:
Before:
element_before_light_c

After:
element_after_light_c

Added 2021-04-01 Before and after demonstrating Replies, mentions, and keywords. (The disappearance of the shield icon is caused by an out-of-date fork)

Dark Theme:
Before:
image

After:
image

Light Theme:
Before:
image

After:
image

I wasn't able to generate a mention without a pill, although it should appear the same as the keyword message does in the examples above.


This PR currently has none of the required changelog labels.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

@SimonBrandner
Copy link
Contributor

@nhhollander, thanks! Could you please include a screenshot so it's easier to review to the visual side?

@nhhollander
Copy link
Author

@SimonBrandner Updated, thanks for the tip

@jryans jryans requested a review from a team March 30, 2021 10:32
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

thanks for taking a look at this! It's a very complex space to review: can we also get screenshots for what user mentions, keyword mentions, etc all look like? These are the same concerns with the previous PR.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks for adding the screenshots. I gave this a quick test and found that keyword (or non-pill) notifications don't seem to work correctly, but I'm not sure that's a major issue. Keywords are incredibly hard to detect in our current notifications system, so it is likely an impossible feat to try and consider them.

This is what I mean:
image

The third and seventh messages are "keyword" notifications, effectively. They don't have a highlight to show what pinged the user, but the yellow background might be enough?

@matrix-org/design what are your thoughts on this? I'm fine with the code changes as-is if the caveat is acceptable.

@turt2live turt2live requested a review from a team April 9, 2021 01:50
@nhhollander
Copy link
Author

Looking at discord, it seems that in addition to the highlight, they have a solid bar of color on the left side to increase visibility. When matrix is in dark mode, the contrast from the highlight alone is pretty visible, but in light mode it gets kind of washed out. I'll give that shot and see how it looks.

@nhhollander
Copy link
Author

It appears at first glance that this issue has been fixed (along with a bunch of other improvements) in #3553.

@robintown
Copy link
Member

@nhhollander I don't believe element-hq/element-web#3553 covered most of the changes in this PR. Messages are still entirely highlighted red when they ping you, that's just now with the exception of quoted messages in replies.

@nhhollander
Copy link
Author

Oops, looks like I misread that PR!

@nhhollander nhhollander reopened this Jul 15, 2021
@haydenmc
Copy link

haydenmc commented Oct 4, 2021

Hi friends! Been a few months - any chance of this getting merged? Seems like low hanging fruit!

@nhhollander
Copy link
Author

@haydenmc I lost my dev environment when I moved computers, but I'll try to pull these changes with the latest release and see if it still works sometime this week. There's been loads of changes over the last few months, I haven't gotten a chance to check if there are any potentially conflicting changes.

@mdsina
Copy link

mdsina commented Mar 8, 2022

Any progress on this?

@MadLittleMods MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Apr 29, 2022
@niquewoodhouse
Copy link
Contributor

I'd like to review this on behalf of design but can't find the view deployment option

@SimonBrandner
Copy link
Contributor

@nhhollander, could you please merge develop?

@robintown
Copy link
Member

I've created #9199 to replace this PR and allow the design team to take a look.

@robintown robintown closed this Aug 17, 2022
@newdawncrypto
Copy link

When will the hideous red color be removed and replaced? It's been years...

@turt2live
Copy link
Member

Please stop posting in multiple places. One comment is more than enough to get the attention of the people who can help you.

@matrix-org matrix-org locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants