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
Details
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
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
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