Details

Reviewers

tomek
atul
• abosh
ginsu

Commits

rCOMM9dbbedf7d7e4: [web] Use Button for text buttons

Summary

Part of ENG-843
Adds a new 'text' variant to Button and uses it instead of plain html elements in:

settings screen
retry send button
user list in thread composer

to keep the web app more consistent.

This diff also fixes visual bugs with retry message:
Before:

After:

Test Plan

Check if

settings screen
retry send button
user list in thread composer

look and work correctly.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

michal created this revision.Oct 10 2022, 2:49 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 10 2022, 2:49 AM

michal added a parent revision: D5321: [web] Separate color in Button.Oct 10 2022, 2:57 AM

michal added a child revision: D5323: [web] Use Button in join chat button.

Harbormaster completed remote builds in B12684: Diff 17434.Oct 10 2022, 2:59 AM

michal requested review of this revision.Oct 10 2022, 2:59 AM

ashoat added a reviewer: ginsu.Oct 10 2022, 8:40 AM

Accepting with questions inline

web/chat/chat-message-list.css
157 ↗	(On Diff #17434)	Can you provide some context on this change?
web/chat/failed-send.react.js
94 ↗	(On Diff #17434)	`css.retryButtonText` applies `text-transform: uppercase` Instead of doing that in CSS can we do it here? Just change `Retry?` to `RETRY?`

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

(At a high level, would be good to provide a bit more context in the Description section to make things a bit easier for reviewers.)

There's a css block:

div.failedSend {
  text-transform: uppercase;
  (...)
}

that previously applied to the "Retry?", but now that we use Button it had to be copied to retryButtonText. So I just kept the old behaviour, but I don't see any reason why we couldn't just use RETRY? (and <span>DELIVERY FAILED.</span>).

michal planned changes to this revision.Oct 11 2022, 1:16 AM

michal edited the summary of this revision. (Show Details)

Removed the text-transform: uppercase. Fixed a bug with buttons in thread composer and this resulted in a slight change in visuals (see the updated summary). But that could be easily changed if it's undesirable.

This revision is now accepted and ready to land.Oct 11 2022, 5:58 AM

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

michal edited the summary of this revision. (Show Details)

michal edited the test plan for this revision. (Show Details)

michal attached a referenced file: F195909: Screen Recording 2022-10-11 at 14.45.55.mov. (Show Details)

Harbormaster completed remote builds in B12725: Diff 17486.Oct 11 2022, 6:08 AM

Is text-transform: uppercase better for accessibility than just making the text all-caps? I'm not sure if it is, but I think that was the main reason for doing that

According to this article it's better for screen-readers (and it's better for a few other reasons but I don't think they are applicable to our example). I think we should change it back to text-transform.

Cool let's do that, I agree

Sounds good, thanks for explaining + linking to the article.

This revision is now accepted and ready to land.Oct 11 2022, 7:31 AM

Looks good, I would just address the text-transform: uppercase comments before landing

Added text-transform: uppercase back. This update also fixes two bugs that introduce changes to visuals so I'm requesting a review again.

the RETRY? button didn't change color depending on chat color (I didn't notice it before because I was testing on a blue chat)
The DELIVERY FAILED text was black so it wasn't visible. I wanted to create a task for this later, but while adding text-transform back I think it made sense to do this here instead of a one-line change later.

Please see the summary for images.

Harbormaster failed remote builds in B12739: Diff 17501!Oct 12 2022, 1:41 AM

michal edited the summary of this revision. (Show Details)Oct 12 2022, 1:44 AM

michal edited the test plan for this revision. (Show Details)

michal requested review of this revision.Oct 12 2022, 1:46 AM

Harbormaster completed remote builds in B12739: Diff 17501.Oct 12 2022, 6:08 AM

atul accepted this revision.Oct 12 2022, 9:47 AM

This revision is now accepted and ready to land.Oct 12 2022, 9:47 AM

Update. After changes to :hover behaviour in the newest revision of the previous diff, there are no longer visual changes in the thread composer.