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

Changing the color of highlights to avoid confusions #2896

Closed
wants to merge 1 commit into from

Conversation

heIsThePirate
Copy link

(with the aim of resolving issues #9070 and related)
From:
Old

to:

New

Signed-off-by: Mohit Singh mohitsingh1997@gmail.com

Signed-off-by: Mohit Singh <mohitsingh1997@gmail.com>
@turt2live turt2live requested a review from a team April 8, 2019 21:29
turt2live
turt2live previously approved these changes Apr 8, 2019
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 doing this! It looks like the right option in general, however I do wonder how it handles keyword or plaintext notifications. Some clients (namely our own mobile clients, embarrassingly) still don't send pills when mentioning people, which may cause this to unintentionally break. If you've tested this, please include screenshots :)

@bmarty
Copy link
Contributor

bmarty commented Apr 9, 2019

FTR RiotX will send pills

@heIsThePirate
Copy link
Author

heIsThePirate commented Apr 9, 2019

Thanks for the approval and the feedback @turt2live
I have tested this in a dev environment set up on my PC for the 'riot-web' and the above screenshots in my original post are for the same. Now, as @bmarty has pointed out that 'RiotX' will send pills and according to the announcement, the codebase of 'riot-android' would be replaced with that of 'RiotX', I don't think that would be a problem anymore...In the meanwhile, I think the stylesheet is already overwritten for the mobile client as this is what I observe in the 'riot-android' for the same content: https://ibb.co/zmB6W5P
I am not much familiar with Android still, I would try to test it with the gradle and update if any issues are found.

@jryans
Copy link
Collaborator

jryans commented Apr 9, 2019

Riot iOS does not appear to send pills at the moment.

@turt2live
Copy link
Member

Keyword notifications are also not pills, so this is still a real concern.

@jryans
Copy link
Collaborator

jryans commented Apr 9, 2019

@turt2live So does it mean we're not ready to merge a change like this, then?

@turt2live
Copy link
Member

If the message isn't highlighted when there's no pills, I'm afraid so.

Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

From the PR discussion, it sounds like we need to find a different approach that still highlights even without pills.

@turt2live turt2live dismissed their stale review April 9, 2019 16:13

doesn't apply

@turt2live
Copy link
Member

@heIsThePirate just checking in - are you able to take this on still? If not, we can add it to our backlog.

@heIsThePirate
Copy link
Author

@heIsThePirate just checking in - are you able to take this on still? If not, we can add it to our backlog.

I have tried several things but wasn't able to test properly due to the recent chaos, this will take some more time. I am busy with exams right now so I guess I would take this on again in a week. Until then, if required, it may be added to the backlog. Thanks!

@jryans
Copy link
Collaborator

jryans commented Apr 24, 2019

Since we have some unresolved concerns (which I am not quite sure how we would resolve at the moment), and also the author is busy for now, I think it's best to close this for now. Please feel free to open a new PR if it would address the concerns highlighted here. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants