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
Details
Details
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:
After:
Update: The colors of the buttons are the same; however, there are no more icons
Add friend and block case
Withdraw friend request case
Accept and reject friend request case
unfriend case
unblock case
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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:
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.
- Just update the color (we should be able to land that pretty much immediately)
- 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): 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 |
Comment Actions
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 |