Page MenuHomePhabricator

[web] [fix] hover state background color on thread list
ClosedPublic

Authored by benschac on Mar 14 2022, 9:32 AM.
Tags
None
Referenced Files
F3378750: D3421.id10585.diff
Wed, Nov 27, 12:49 PM
Unknown Object (File)
Sat, Nov 2, 8:04 AM
Unknown Object (File)
Tue, Oct 29, 12:13 AM
Unknown Object (File)
Tue, Oct 29, 12:13 AM
Unknown Object (File)
Oct 21 2024, 2:12 AM
Unknown Object (File)
Oct 20 2024, 9:43 PM
Unknown Object (File)
Oct 20 2024, 2:08 AM
Unknown Object (File)
Oct 19 2024, 11:48 PM

Details

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Huh it looks like this is what it is in the design, but it seems odd to me that the hover background color is so much brighter than the active thread background color.

Accepting but adding @ashoat as blocking reviewer just to give it another look

This revision is now accepted and ready to land.Mar 14 2022, 5:26 PM
This revision now requires review to proceed.Mar 14 2022, 5:26 PM
ashoat requested changes to this revision.Mar 14 2022, 9:48 PM

I agree this looks weird, maybe we should flip them, so that the selected state is higher-contrast than the hover state? What do you all think?

This revision now requires changes to proceed.Mar 14 2022, 9:48 PM

Screen Recording 2022-03-15 at 09.27.21 AM.gif (795×1 px, 1 MB)

I flipped the colors. I don't think this will really work with the updated mark and unread styling.

Marking this diff as Needs Review to get it in your queue ashoat even though there aren't code changes. Just want to make sure you see this gif / screenshot.

ashoat requested changes to this revision.Mar 15 2022, 12:53 PM

Looks good to me!

I don't think this will really work with the updated mark and unread styling.

Not sure what you're referring to, or what the issue would be. Can you clarify?

Back to you to make changes

This revision now requires changes to proceed.Mar 15 2022, 12:53 PM
In D3421#93110, @ashoat wrote:

Looks good to me!

I don't think this will really work with the updated mark and unread styling.

Not sure what you're referring to, or what the issue would be. Can you clarify?

Back to you to make changes

Adding a screenshot here.

Clarifying:

Image 2022-03-16 at 10.23.08 AM.jpg (1×1 px, 119 KB)

  • The mark as unread background will be the same color as the background of the hover state if we make your proposed change

I agree this looks weird, maybe we should flip them, so that the selected state is higher-contrast than the hover state? What do you all think?

Image 2022-03-16 at 10.27.03 AM.jpg (864×2 px, 114 KB)

  • If we stick with the design from figma. The mark as unread button has a clear background and doesn't blend into the background of the conversation item.

Followed up with a comment about how this color switch will adversely effect the mark as unread button.

Lets let IRL if there's still confusion and avoid churn.

ashoat requested changes to this revision.Mar 16 2022, 8:56 PM

Ah yeah, I can see that design doesn't look good, with the mark-as-unread background being the same as the ChatThreadListItem background. What I don't understand, is that my suggestion was just to flip hover and selected state... so are you're saying that in the Figma design, one of either the hover state or the selected state has the same color as the mark-as-unread background? Or does the Figma design simply not cover both hover state and selected state?

If the Figma design only has a design for one, then we can just make the hover state and the selected state the same. If it has a design for both, but one of them has the same background as the mark-as-unread button, then we'll need to figure out a new design.

This revision now requires changes to proceed.Mar 16 2022, 8:56 PM
In D3421#93639, @ashoat wrote:

Ah yeah, I can see that design doesn't look good, with the mark-as-unread background being the same as the ChatThreadListItem background. What I don't understand, is that my suggestion was just to flip hover and selected state... so are you're saying that in the Figma design, one of either the hover state or the selected state has the same color as the mark-as-unread background? Or does the Figma design simply not cover both hover state and selected state?

If the Figma design only has a design for one, then we can just make the hover state and the selected state the same. If it has a design for both, but one of them has the same background as the mark-as-unread button, then we'll need to figure out a new design.

my suggestion was just to flip hover and selected state... so are you're saying that in the Figma design, one of either the hover state or the selected state has the same color as the mark-as-unread background?

Yes. If we flip the colors per your suggestion. the selected state will have the same background as mark as unread causing a visual regression. If we leave the selected color and hover color as they are in figma, we will not have an issue.

To be even more explicit:

  • mark as unread background: Shades of Black / 90
  • hover background: Shades of Black / 90
  • selected background: Shades of Black / 80

If we flip the hover and selected background, mark as unread background will be the same color as the selected background.

Or does the Figma design simply not cover both hover state and selected state?

Figma covers both, the flipped color solution will break the visual separation of mark as unread.

If it has a design for both, but one of them has the same background as the mark-as-unread button, then we'll need to figure out a new design.

If we look at

slack:

Image 2022-03-17 at 10.18.26 AM.jpg (1×1 px, 209 KB)

signal:
Image 2022-03-17 at 10.17.39 AM.jpg (2×2 px, 286 KB)

telegram:
Image 2022-03-17 at 10.24.18 AM.jpg (1×2 px, 283 KB)

discord (but it's less pronounced):
Image 2022-03-17 at 10.22.40 AM.jpg (1×1 px, 353 KB)

All follow this same pattern, the selected item is bright, hovered item is dim.
I really think we should stick with this same pattern defined in figma: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1085%3A85147

No code change, but setting to needs review to get this on ashoat's queue.

ashoat requested changes to this revision.Mar 17 2022, 12:37 PM

Talked about this in person, @benschac is right in pointing out that flipping this will make the design of the mark-as-unread button look weird. The reason the original design from Alicja doesn't have this problem is that there is no way to have mark-as-unread appear when a ChatThreadListItem isn't in the hover state. If the user's pointer leaves ChatThreadListItem, then the mark-as-unread button disappears – which avoids a scenario where the mark-as-unread-button is laid over the selected state.

That said, I still feel strongly that Alicja's decision to make hover state higher-contrast than selected state is weird. The designs @benschac shared above reinforce this – in all of the examples, selected state is higher-contrast than hover state.

I still think we should flip the designs for hover state and selected state here. To address the issue with the mark-as-unread button having the same background color, I think we should just change the colors for the mark-as-unread (so that they don't actually match Alicja's design).

This revision now requires changes to proceed.Mar 17 2022, 12:37 PM

update hover and selected state

Screen Recording 2022-03-21 at 05.38.50 PM.gif (18 MB)

Adding a gif here @ashoat -- I'm pretty sure we're on the same page with UX / what the selected color / hover color should be but just want to be sure.

web/chat/chat-thread-list.css
28 ↗(On Diff #10585)

I'm not 100% on this.

benschac added inline comments.
web/chat/chat-thread-list.css
31 ↗(On Diff #10585)

introducing :is to the code base: https://css-tricks.com/almanac/selectors/i/is/

The gif doesn't cover the mark-as-unread button. Can you share a screenshot to confirm that the contrast looks good?

In D3421#94707, @ashoat wrote:

The gif doesn't cover the mark-as-unread button. Can you share a screenshot to confirm that the contrast looks good?

Yes, I'll update the mark as unread diff though (which is in this stack).

This diff is specifically addressing the hover and selection states.

D3422 handles the mark as unread styling.

Accepting based on the animated GIF above. The screenshots in the diff description still seem wrong, but I think those are outdated? Might be good to update the diff description (replace old screenshots with new animated GIF), so that the Git commit (once landed) has the correct visual

This revision is now accepted and ready to land.Mar 22 2022, 8:22 AM