Move all the compose meta data to be above the compose input #9095

tgolen · 2022-05-19T16:18:16Z

Details

Some of the information used to be above the compose, and some of it below. All of them had inconsistent styling and bad practices for layout out the UI components.

I put the "user is typing" block at the end, since that's the one that has the biggest chance of flashing on and off. Putting it at the end makes it the least jarring so the position of the other elements don't change when it flashes on and off.

Fixed Issues

$ #9087

Tests & QA

Open a chat
Go offline
Verify the offline message now appears above the compose input
Go back online
Compose a message with more than 60,000 characters (use copy/paste to do this easily)
Verify the comment exceeded meta data is displayed above the compose input
Open a second incognito browser and sign in as the other chat participant and start typing
Verify that the "user is typing" message is above the compose input
Open a third incognito browser and sign in as a third chat participant
Start a group chat with all three people
Quickly type something in the compose input in two of the browsers
Verify that in the third browser the "multiple people are typing" appears and it's displayed above the compose input
Verify that all the vertical and horizontal spacing looks OK

Verify that no errors appear in the JS console

Screenshots

**NOTE: For these screenshots, I hacked the code to display all pieces of meta data at once. This will not be possible to do in production because for example, you can't see "user is typing" when you're offline. **

Multiple users typing:

2022-05-19_10-12-44.mp4

Web

Mobile Web

Desktop

iOS

Android

tgolen · 2022-05-19T16:20:04Z

cc @Luke9389 I created the <YouAppearToBeOffline> component in this PR. I think you were looking to created that from your design doc, but I just couldn't resist doing it here since it cleaned up the report compose view so well.

Luke9389 · 2022-05-19T20:11:43Z

OK cool! Thanks for the heads up

amyevans

Just a small comment on styling

amyevans · 2022-05-19T20:25:34Z

src/pages/home/report/ReportActionCompose.js

+                    {shouldShowReportRecipientLocalTime
+                        && (
+                            <View style={[styles.mr3]}>
+                                <ParticipantLocalTime participant={reportRecipient} />
+                            </View>
+                        )}
+
+                    <View style={[styles.mr3]}>
+                        <YouAppearToBeOffline />
+                    </View>
+
+                    <View style={[styles.mr3]}>
+                        <ExceededCommentLength commentLength={this.comment.length} />
+                    </View>
+
+                    <View style={[styles.mr3]}>
+                        <ReportTypingIndicator reportID={this.props.reportID} />
+                    </View>


Wrapping each of these components in a <View style={[styles.mr3]}> is going to produce inconsistent margins when, for example, the first and last components are visible, but the 2nd and 3rd return null. I think it'd be better to apply a gap: 12px; style to the flex container and eliminate these inner wrappers.

Also, NAB, but I think it reads cleaner to have && ( on the same line as the boolean var: {shouldShowReportRecipientLocalTime && (

Oh, that's a really good point with the spacing. I like the idea of using gap!

Luke9389

This all looks good to me 👍

Luke9389 · 2022-05-23T22:32:52Z

src/components/YouAppearToBeOffline.js

+    ...withLocalizePropTypes,
+};
+
+const YouAppearToBeOffline = (props) => {


There was consensus in the Design Doc to call this component OfflineIndicator because that describes what it is, not what it says or does.

Hm, OK. I personally feel like that is not really a great name for it because an "offline indicator" could be done in several different ways (just an icon, just text, a spinner, something else, etc.). I do not feel too strongly about it at this point though, so I will change the name.

That's a good point. I'm also not dug in on OfflineIndicator either, just going by the threads in the doc. I think @iwiznia was more strongly opinionated about it. Thanks for making the change anyway. Down to rename if we re-discuss and end up switching back.

YouAppearToBeOffline also does not tell you if it is an icon, just text, a spinner, something else, no?

Yeah, right after I said that, I realized the name I had wasn't much better.

tgolen · 2022-05-24T15:13:54Z

Updated

tgolen · 2022-05-24T15:14:19Z

Oh, I forgot to make the gap change. Let me do that

tgolen · 2022-05-24T15:16:12Z

Actually, I found that gap is not yet supported in React-Native :( styled-components/styled-components#3628 so I won't be updating it.

Luke9389 · 2022-05-24T17:42:26Z

I'll give @amyevans a chance to check this out one more time, given the RN gap gap.

amyevans · 2022-05-24T21:09:22Z

Actually, I found that gap is not yet supported in React-Native

TIHI 🙃

This is my first time diving into RN styling and I am shocked/frustrated how limited its capabilities are 😬. Though obviously not as elegant, another option would be to pass styles.mr3 to each child component via the style prop and then spread what's received within the children. E.g. <ParticipantLocalTime participant={reportRecipient} style={[styles.mr3]} /> and then https://github.com/Expensify/App/blob/tgolen-move-istyping/src/pages/home/report/ParticipantLocalTime.js#L61 could be style={[styles.chatItemComposeSecondaryRowSubText, ...this.props.style]}

Luke9389 · 2022-05-27T18:32:43Z

Hey @amyevans, it looks like Tim is gonna be OOO until June 6. There's an N07 Issue held on this PR, so would it be OK for us to merge this and then make a separate issue for styling? I'd be happy to follow you over there and be the reviewer if you'd like to implement it (I've got the context and you've got the idea).

amyevans · 2022-05-27T18:38:21Z

Sure, that sounds perfect to me 😄

OSBotify · 2022-05-27T18:49:32Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

thesahindia · 2022-05-28T19:00:48Z

We have extra whitespace below the last message in chats where we aren't showing current time of the contact
(not sure if it's related to this PR)

cc: @tgolen

OSBotify · 2022-05-30T10:51:43Z

🚀 Deployed to staging by @amyevans in version: 1.1.69-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-05-31T15:25:46Z

🚀 Deployed to production by @luacmartins in version: 1.1.69-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

tgolen added 5 commits May 19, 2022 08:58

Restructure the typing indicator a little better

85e8698

Add new component for appearing to be offline

63b5fb4

Add new component for exceeding comment length

6aa89a0

Move layout styles to the parent

67825d9

Remove some debugging code

578e793

tgolen requested a review from a team as a code owner May 19, 2022 16:18

tgolen self-assigned this May 19, 2022

melvin-bot bot requested review from amyevans and removed request for a team May 19, 2022 16:18

Simplify the comment exceeded logic

badf52c

tgolen requested a review from Luke9389 May 19, 2022 16:20

Merge branch 'main' into tgolen-move-istyping

0846c9d

amyevans reviewed May 19, 2022

View reviewed changes

Luke9389 approved these changes May 19, 2022

View reviewed changes

Luke9389 suggested changes May 23, 2022

View reviewed changes

tgolen added 3 commits May 24, 2022 09:09

Merge branch 'main' into tgolen-move-istyping

231f161

Rename component

657c286

Rename file and import

120803d

Luke9389 approved these changes May 24, 2022

View reviewed changes

amyevans approved these changes May 27, 2022

View reviewed changes

amyevans merged commit 70479c3 into main May 27, 2022

amyevans deleted the tgolen-move-istyping branch May 27, 2022 18:40

amyevans mentioned this pull request May 27, 2022

Fix margins in compose metadata area #9205

Closed

OSBotify mentioned this pull request May 30, 2022

Deploy Checklist: New Expensify 2022-05-30 #9217

Closed

22 tasks

mountiny mentioned this pull request May 31, 2022

Padding seems off for the latest message with the typing indicator moved to the top #9251

Closed

mountiny mentioned this pull request Jun 2, 2022

Move the user is typing and other metadata back below the composer box #9276

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all the compose meta data to be above the compose input #9095

Move all the compose meta data to be above the compose input #9095

tgolen commented May 19, 2022

tgolen commented May 19, 2022

Luke9389 commented May 19, 2022

amyevans left a comment

amyevans May 19, 2022

tgolen May 19, 2022

Luke9389 left a comment

Luke9389 May 23, 2022

tgolen May 24, 2022

Luke9389 May 24, 2022

iwiznia May 24, 2022

tgolen May 24, 2022

tgolen commented May 24, 2022

tgolen commented May 24, 2022

tgolen commented May 24, 2022

Luke9389 commented May 24, 2022

amyevans commented May 24, 2022

Luke9389 commented May 27, 2022

amyevans commented May 27, 2022

OSBotify commented May 27, 2022

thesahindia commented May 28, 2022

OSBotify commented May 30, 2022

OSBotify commented May 31, 2022

Move all the compose meta data to be above the compose input #9095

Move all the compose meta data to be above the compose input #9095

Conversation

tgolen commented May 19, 2022

Details

Fixed Issues

Tests & QA

Screenshots

Web

Mobile Web

Desktop

iOS

Android

tgolen commented May 19, 2022

Luke9389 commented May 19, 2022

amyevans left a comment

Choose a reason for hiding this comment

amyevans May 19, 2022

Choose a reason for hiding this comment

tgolen May 19, 2022

Choose a reason for hiding this comment

Luke9389 left a comment

Choose a reason for hiding this comment

Luke9389 May 23, 2022

Choose a reason for hiding this comment

tgolen May 24, 2022

Choose a reason for hiding this comment

Luke9389 May 24, 2022

Choose a reason for hiding this comment

iwiznia May 24, 2022

Choose a reason for hiding this comment

tgolen May 24, 2022

Choose a reason for hiding this comment

tgolen commented May 24, 2022

tgolen commented May 24, 2022

tgolen commented May 24, 2022

Luke9389 commented May 24, 2022

amyevans commented May 24, 2022

Luke9389 commented May 27, 2022

amyevans commented May 27, 2022

OSBotify commented May 27, 2022

thesahindia commented May 28, 2022

OSBotify commented May 30, 2022

OSBotify commented May 31, 2022