Page MenuHomePhabricator

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

Authored by benschac on Mar 23 2022, 10:21 AM.
Tags
None
Referenced Files
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
Unknown Object (File)
Oct 7 2024, 5:29 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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.