Page MenuHomePhabricator

[web] Remove hover tint from relationship buttons
AbandonedPublic

Authored by michal on Oct 11 2022, 7:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 14, 4:49 PM
Unknown Object (File)
Thu, Nov 14, 4:48 PM
Unknown Object (File)
Thu, Nov 14, 4:35 PM
Unknown Object (File)
Oct 15 2024, 9:49 AM
Unknown Object (File)
Oct 15 2024, 9:48 AM
Unknown Object (File)
Oct 15 2024, 9:29 AM
Unknown Object (File)
Sep 28 2024, 6:39 AM
Unknown Object (File)
Sep 14 2024, 3:22 PM
Subscribers

Details

Reviewers
tomek
atul
abosh
ginsu
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Raised in this comment, during work on the ENG-843
Removes the hover tint from buttons in the relationship tab in chat settings, to make it more consistent with relationship buttons displayed at the top of the chat.

Visual changes: (I couldn't capture the cursor, so just know that the cursor in both pictures is hovering over the green button)
Before:

image.png (500×740 px, 42 KB)

After:
image.png (478×752 px, 40 KB)

Test Plan

Test if the hover tint is gone. Test if the buttons still work.

Diff Detail

Repository
rCOMM Comm
Branch
relationship-button-consistency
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

michal attached a referenced file: F195779: image.png. (Show Details)
michal edited the summary of this revision. (Show Details)
ginsu added a reviewer: Restricted Owners Package.Oct 11 2022, 12:22 PM
ginsu requested changes to this revision.Oct 11 2022, 12:25 PM

I personally think we should have some sort of tint for the hover, the user experience feels kinda weird without it. Will defer to a blocking reviewer for more feedback on this

web/modals/threads/settings/thread-settings-relationship-button.react.js
70–73

nice catch!

This revision now requires changes to proceed.Oct 11 2022, 12:25 PM

Kind of agree, although I also agree with @atul that the green tint is a bit too dark.

I've just checked the code and the success Button variant (the green one) is only used in the context of relationship buttons. We could easily change the hover tint (var(--btn-bg-success-hover)) to a lighter one and keep everything as is but I don't know if it's a correct solution.

I just had a 1:1 with @michal where we talked about this. Here are some thoughts:

  1. I think it's important that we have a consistent style for all the buttons in the web app. I think we're all in agreement that this is the priority
  2. I think the hover effect is nice and it would be good to keep it. But if we keep it, we should make sure it works for all variants, and not just success and danger
  3. I agree that the hover effect on the success variant is currently too "extreme"

With that context, my suggestion is to make the hover work by modifying the base color in some way. It seems like the best way to do this is with the CSS filter property, but there are other ways I'm sure (eg. second layer in front, CSS functions, or worst case doing it in JS).

Abandoning the diff in favour of other solutions.