Page MenuHomePhabricator

[web] Change See More... hover color to match chat threads hover color
ClosedPublic

Authored by rohan on Sep 16 2022, 10:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 6:26 AM
Unknown Object (File)
Oct 16 2024, 4:02 AM
Unknown Object (File)
Oct 13 2024, 12:04 PM
Unknown Object (File)
Oct 11 2024, 2:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM
Unknown Object (File)
Oct 10 2024, 3:16 PM

Details

Summary

Changed the div.thread class's hover color to match the hover color of the
activeThread, since var(--thread-active-bg) is a different shade from
var(--thread-hover-bg).

The difference is shown in the themes.css file:

--thread-hover-bg: var(--shades-black-90);
--thread-active-bg: var(--shades-black-80);
Test Plan

Ran the before and after versions of this change and visually
the colors are the same with this change.

Before:

Screen Shot 2022-09-16 at 1.46.50 PM.png (422×802 px, 37 KB)

After:

Screen Shot 2022-09-16 at 1.46.13 PM.png (402×810 px, 36 KB)

For a closer look at the change in the thread color:

Before:

Screen Shot 2022-09-16 at 2.10.01 PM.png (410×816 px, 36 KB)

After:

Screen Shot 2022-09-16 at 2.09.50 PM.png (440×812 px, 37 KB)

Diff Detail

Repository
rCOMM Comm
Branch
ENG-1462
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan edited the summary of this revision. (Show Details)
atul requested changes to this revision.Sep 21 2022, 7:33 AM

I don't think we should be changing the background for div.thread:hover selector from --thread-hover-bg (which based on the name is clearly the right variable) to --thread-active-bg.

There are no other usages of --thread-hover-bg so if anything we should be changing the value in theme.css.

Can you take another look to see if there's a better approach here?

This revision now requires changes to proceed.Sep 21 2022, 7:33 AM

Sure, that makes sense. For the reason that --thread-hover-bg isn't used anywhere else I thought it made sense just to change the background from --thread-hover-bg to --thread-active-bg. Though I agree that it makes sense to change --thread-hover-bg directly in theme.css.

  1. Updating D5162: [web] Change See More... hover color to match chat threads hover color #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment.

Revision to update themes.css file
Updates the theme.css file for the --thread-hover-bg value instead of changing the css corresponding to the element.

Revised #1.png (1×802 px, 108 KB)

Revised #2.png (1×814 px, 109 KB)

This revision is now accepted and ready to land.Sep 21 2022, 12:02 PM