Page MenuHomePhabricator
Differential D5344

[web] Remove hover tint from relationship buttons
AbandonedPublic
Actions

Authored by michal on Oct 11 2022, 7:59 AM.
Tags
None
Referenced Files
F3531596: D5344.diff
Wed, Dec 25, 7:49 AM
Unknown Object (File)
Wed, Dec 18, 11:00 AM
Unknown Object (File)
Wed, Dec 18, 10:58 AM
Unknown Object (File)
Wed, Dec 18, 10:46 AM
Unknown Object (File)
Mon, Nov 25, 10:34 PM
Unknown Object (File)
Mon, Nov 25, 8:18 PM
Unknown Object (File)
Mon, Nov 25, 8:17 PM
Unknown Object (File)
Nov 14 2024, 4:49 PM
View All 92 Files
Subscribers
ashoat

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 created this revision.Oct 11 2022, 7:59 AM
Herald added a subscriber: ashoat. · View Herald TranscriptOct 11 2022, 7:59 AM
michal edited the summary of this revision. (Show Details)Oct 11 2022, 8:01 AM
michal attached a referenced file: F195779: image.png. (Show Details)
michal edited the summary of this revision. (Show Details)
michal added a parent revision: D5324: [web] Use Button for relationship buttons.
Harbormaster completed remote builds in B12731: Diff 17492.Oct 11 2022, 8:10 AM
michal requested review of this revision.Oct 11 2022, 8:10 AM
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
michal added a comment.Oct 12 2022, 1:19 AM

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.

ashoat added a comment.Oct 13 2022, 8:17 AM

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).

michal abandoned this revision.Oct 14 2022, 8:09 AM

Abandoning the diff in favour of other solutions.

Revision Contents
Changeset List

PathSize
web/
modals/
threads/
settings/
25 lines

Diff 17492

View Options

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

Loading...