Page MenuHomePhabricator

[web] Standardize spoiler color
ClosedPublic

Authored by rohan on Dec 6 2022, 4:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 12:05 PM
Unknown Object (File)
Sat, Dec 7, 1:34 PM
Unknown Object (File)
Sat, Dec 7, 1:33 PM
Unknown Object (File)
Sat, Dec 7, 1:31 PM
Unknown Object (File)
Sat, Dec 7, 1:19 PM
Unknown Object (File)
Wed, Dec 4, 3:41 AM
Unknown Object (File)
Nov 19 2024, 2:22 PM
Unknown Object (File)
Nov 19 2024, 2:22 PM
Subscribers

Details

Summary

Two things here - the first is that we probably want to make the color of spoilers the same as it is on native, #33332C. The second thing is the current spoiler color on received messages is impossible to differentiate from the message bubble.

Test Plan

Will link before/after on web

BeforeAfter
Screenshot 2022-12-06 at 7.35.33 PM.png (1×2 px, 239 KB)
Screenshot 2022-12-06 at 7.35.43 PM.png (1×2 px, 237 KB)
Screenshot 2022-12-06 at 7.49.15 PM.png (1×2 px, 198 KB)
Screenshot 2022-12-06 at 7.49.34 PM.png (1×2 px, 217 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan requested review of this revision.Dec 6 2022, 4:52 PM

Looks good, thanks for including the screenshots in the Test Plan (everyone is doing these fancy markdown tables now huh)

This revision is now accepted and ready to land.Dec 6 2022, 5:51 PM
This revision was automatically updated to reflect the committed changes.

We should prefer using something from our existing color palette - the first section of this file. If we really have to introduce a new color, we should introduce it in the first section and use here.

In D5836#175504, @tomek wrote:

We should prefer using something from our existing color palette - the first section of this file. If we really have to introduce a new color, we should introduce it in the first section and use here.

Thanks for pointing this out! I switched from var(--shades-black-80) to #33332c to keep the color consistent with native, and also more importantly to prevent the spoilers from blending in with other colors and making it difficult to see.

I can put up a super quick change for this if you think keeping #33332c is fine and move --spoiler-text-color: #33332c and --spoiler-background-color: #33332c up to after line 52

In D5836#175920, @rohan wrote:
In D5836#175504, @tomek wrote:

We should prefer using something from our existing color palette - the first section of this file. If we really have to introduce a new color, we should introduce it in the first section and use here.

Thanks for pointing this out! I switched from var(--shades-black-80) to #33332c to keep the color consistent with native, and also more importantly to prevent the spoilers from blending in with other colors and making it difficult to see.

I can put up a super quick change for this if you think keeping #33332c is fine and move --spoiler-text-color: #33332c and --spoiler-background-color: #33332c up to after line 52

There are a lot of options here, but most of them aren't worth it. A good enough solution for now might be to create a new variable, just as you suggested, and use it here.