Page MenuHomePhabricator

[web] Introduce colors for relationship action buttons
ClosedPublic

Authored by ginsu on Sep 9 2022, 8:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 17, 3:25 PM
Unknown Object (File)
Sat, Nov 9, 9:26 PM
Unknown Object (File)
Sat, Nov 9, 7:40 PM
Unknown Object (File)
Sat, Nov 9, 7:29 PM
Unknown Object (File)
Sat, Nov 9, 5:49 PM
Unknown Object (File)
Sat, Nov 9, 5:43 PM
Unknown Object (File)
Sat, Nov 9, 5:30 PM
Unknown Object (File)
Sat, Nov 9, 3:46 PM

Details

Summary

The buttons in the relationship chat settings now have colors that are consistent with the other relationship prompt buttons found in the app

Depends on D5088

Test Plan

Please view the screenshots to see the before and after of the changes I made:

Before:
All buttons in the relationship chat settings modal looked like this:

Screen Shot 2022-09-09 at 8.13.55 PM.png (1×850 px, 52 KB)

After:
Update: The colors of the buttons are the same; however, there are no more icons

Screen Shot 2022-09-14 at 11.08.24 PM.png (938×868 px, 51 KB)

Add friend and block case

Screen Shot 2022-09-10 at 12.27.04 AM.png (1×886 px, 71 KB)

Withdraw friend request case

Screen Shot 2022-09-10 at 12.27.37 AM.png (1×1 px, 108 KB)

Accept and reject friend request case

Screen Shot 2022-09-10 at 12.28.02 AM.png (1×1 px, 97 KB)

unfriend case

Screen Shot 2022-09-10 at 12.28.08 AM.png (1×898 px, 70 KB)

unblock case

Screen Shot 2022-09-10 at 12.28.14 AM.png (1×1 px, 88 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Sep 9 2022, 8:50 AM
ginsu retitled this revision from introduced icons and colors for relationship action buttons to [web] introduced icons and colors for relationship action buttons.Sep 11 2022, 8:39 AM
atul retitled this revision from [web] introduced icons and colors for relationship action buttons to [web] Introduce icons and colors for relationship action buttons.Sep 12 2022, 10:27 AM

updated diff to use new success variant logic in button component

atul requested changes to this revision.Sep 13 2022, 11:49 AM

Thanks for including all of those screenshots in the Test Plan!

One thing I noticed is that the icons aren't aligned and their position varies pretty wildly:

Screen Shot 2022-09-13 at 2.44.28 PM.png (784×1 px, 86 KB)

There are a lot of ways we can use flex box, etc to lay this out in a way that looks cleaner (eg flex-row with an icon pushed to the left and text centered within a container for remaining width).

What we can do is break this into two separate diffs.

  1. Just update the color (we should be able to land that pretty much immediately)
  2. Include the icon and position the contents of the button in a way that looks better/more consistent irrespective of the length of button text
web/modals/threads/settings/thread-settings-relationship-button.react.js
57–90 ↗(On Diff #16623)

Here's a recent discussion on using switch-case (from https://phab.comm.dev/D4825?id=15599#inline-30948, added screenshot because image load reflows page and doesn't take you to relevant comment):
{F168213}


Given the discussion there, and other discussions about codebase style, I think it'd be best to use if statements here instead.

125–127 ↗(On Diff #16623)

Realize this is a common pattern, but the team has decided against ternaries in JSX: https://www.notion.so/commapp/Use-ternary-conditionals-sparingly-f4ba44a10259403592a1d15440a9847e

This revision now requires changes to proceed.Sep 13 2022, 11:49 AM

resovled atuls comments switch-case comments and removed icons

ginsu edited the test plan for this revision. (Show Details)
ginsu edited the test plan for this revision. (Show Details)
ginsu retitled this revision from [web] Introduce icons and colors for relationship action buttons to [web] Introduce colors for relationship action buttons.Sep 14 2022, 7:45 AM
atul requested changes to this revision.Sep 14 2022, 7:59 AM

Looks very close, once we move the buttonVariant stuff outside of the useMemo() we should be good to go!

web/modals/threads/settings/thread-settings-relationship-button.react.js
47–65 ↗(On Diff #16660)

We can move this out of this useMemo()

Since buttonVariant is a string, we actually don't have to memoize it at all: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/

TLDR we only need to memoize objects (which includes eg arrays, functions, etc) to maintain referential equality

This revision now requires changes to proceed.Sep 14 2022, 7:59 AM

moved variant logic out of useMemo()

This revision is now accepted and ready to land.Sep 15 2022, 7:25 AM