Page MenuHomePhabricator

[web] [refactor] css color should be white
ClosedPublic

Authored by benschac on Mar 23 2022, 10:21 AM.
Tags
None
Referenced Files
F3378842: D3500.id10683.diff
Wed, Nov 27, 1:31 PM
F3378462: D3500.diff
Wed, Nov 27, 10:43 AM
Unknown Object (File)
Fri, Nov 15, 9:01 AM
Unknown Object (File)
Mon, Nov 11, 6:51 AM
Unknown Object (File)
Sun, Nov 10, 1:55 AM
Unknown Object (File)
Thu, Nov 7, 6:48 AM
Unknown Object (File)
Sat, Nov 2, 8:04 AM
Unknown Object (File)
Oct 7 2024, 5:29 AM

Details

Summary

color is white, css class should be white. I think this was left over from the theme'ing dark mode changes. More of the thread list item will be updated in coming diffs

Image 2022-03-23 at 1.27.31 PM.jpg (484×982 px, 41 KB)

Test Plan

color in message preview should still be white

Diff Detail

Repository
rCOMM Comm
Branch
fix-hover-background-color
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested changes to this revision.Mar 24 2022, 4:22 AM

Not sure about this change - it is an improvement comparing to black, but still we can make this a lot better. The problem is, when we will be introducing another theme, we're going to change css variables. We have a nice abstraction that we're using --fg instead of white but that doesn't matter when we use it in a .white class. We should instead keep it more general. Ultimately, what is the benefit of having e.g. --fg when we're hardcoding it's value as a name of a class?

The proper solution here would be to have two classes .unread and .read which should assign proper color --fg, --thread-color-read or --thread-from-color-read.

This revision now requires changes to proceed.Mar 24 2022, 4:22 AM
In D3500#95581, @palys-swm wrote:

Not sure about this change - it is an improvement comparing to black, but still we can make this a lot better. The problem is, when we will be introducing another theme, we're going to change css variables. We have a nice abstraction that we're using --fg instead of white but that doesn't matter when we use it in a .white class. We should instead keep it more general. Ultimately, what is the benefit of having e.g. --fg when we're hardcoding it's value as a name of a class?

The proper solution here would be to have two classes .unread and .read which should assign proper color --fg, --thread-color-read or --thread-from-color-read.

I'm with you on using classes that represent state. I initially didn't make this change because I was concerned about the above class names (white, light, and dark being used in many places in the codebase. chat-thread-list.css is used in 9 different components and possibly introducing a regression).

after searching the code base. css.white, css.light, css.dark are only used in message-preview.react.js

Image 2022-03-24 at 11.25.33 AM.jpg (762×2 px, 115 KB)

Image 2022-03-24 at 11.25.41 AM.jpg (680×2 px, 103 KB)

Image 2022-03-24 at 11.25.50 AM.jpg (898×2 px, 116 KB)

So this change wouldn't introduce a regression.

My only note would be if we could make the css naming change here: D3505. The ternary is abstracted into one call and I wouldn't have rebasing conflicts if I made the change in that diff if that's okay?

My only note would be if we could make the css naming change here: D3505. The ternary is abstracted into one call and I wouldn't have rebasing conflicts if I made the change in that diff if that's okay?

Yeah, I think it makes sense.

after searching the code base. css.white, css.light, css.dark are only used in message-preview.react.js

It might be a good idea to also check css['white'] - we're using this syntax in a couple of places

This revision is now accepted and ready to land.Mar 25 2022, 3:09 AM
In D3500#95810, @palys-swm wrote:

My only note would be if we could make the css naming change here: D3505. The ternary is abstracted into one call and I wouldn't have rebasing conflicts if I made the change in that diff if that's okay?

Yeah, I think it makes sense.

after searching the code base. css.white, css.light, css.dark are only used in message-preview.react.js

It might be a good idea to also check css['white'] - we're using this syntax in a couple of places

  • double checked, nothing is coming up for me.