Details

When you have visual changes like this, it's always good to include screenshots with the before/after (or at least after) so reviewers can verify that there are no regressions. In this case I was able to patch the changes in locally to make sure that there are no visual regressions, but just something to keep in mind for next time

This revision is now accepted and ready to land.Oct 10 2022, 10:08 AM

Actually think the green on hover for relationship-button-green is too dark. Is there a way to lessen the "intensity" of tint for hover state?

This revision now requires changes to proceed.Oct 10 2022, 10:37 AM

Sorry, I should have caught that!

Before these changes, the buttons didn't react to hover. We could go back to the previous behavior and instead of using danger and success colors just set the backgroundColor directly.

If we want to keep the hover animation, but change the tint, we would need to add new css classes and pass them to the Button.

Update: I've just noticed that the relationship tab in the chat settings also uses success and danger variants of Button and it also has the same shade of green on hover:

So if we want to keep the buttons consistent, we would need to either keep this diff as it is or make larger changes.

Hey @michal, unfortunately we can't see your upload because Phabricator has a quirk that makes it difficult to attach images/videos... here are the details

Thanks for the help, I think it should be visible now!

michal requested review of this revision.Oct 11 2022, 6:10 AM

We could go back to the previous behavior and instead of using danger and success colors just set the backgroundColor directly.

I think that makes sense, to keep things as they were before.

So if we want to keep the buttons consistent, we would need to either keep this diff as it is or make larger changes.

And for consistency we should change it in Chat Settings as well. We should probably address that in a separate diff.

atul requested changes to this revision.Oct 11 2022, 7:25 AM

This revision now requires changes to proceed.Oct 11 2022, 7:25 AM

Code looks good to me! Might be good to add another diff to the stack address @atul comments

Edit: just saw Atul's most recent comments, I would follow his feedback

Remove different hover behaviour.

Harbormaster completed remote builds in B12730: Diff 17491.Oct 11 2022, 7:55 AM

Looks super close! Let's memoize the buttonColor object

web/chat/relationship-prompt/relationship-prompt-button.js
22–25	Let's pull this object out and memoize it with `React.useMemo(...)`. Further reading: https://blog.isquaredsoftware.com/2020/05/blogged-answers-a-mostly-complete-guide-to-react-rendering-behavior/#component-render-optimization-techniques