fix:synchronization between state in report switch #2166

0xthierry · 2021-03-31T01:35:37Z

Details

Drafted message text should constrained to the user selected when the text was entered into the compose box, draft icon should show only when text exists in the compose box.

Fixed Issues

#2100

Tests

Start a chat with user A
Start a chat with user B
Start writing a message to user A and don't send
Switch to user B and TextInput must be empty and a draft icon must appers in user A

QA Steps

After drafting message text in the compose box for (but prior to sending), text continues to show after clicking into another chat (2 users)

1.Open a chat with User A
2.Enter text into the compose box (but do not send)
3.Switch to chat with User B
4.Notice: Message drafted in compose box to User A continues to show in compose box in chat with User B

After drafting message text in the compose box for (but prior to sending), text continues to show after clicking into another chat (3 users)

1.Open a chat with User A
2.Enter text into the compose box (but do not send)
3.Switch to chat with User B
4.Switch to a chat with User C
5.Notice: Message drafted in compose box to User A continues to show in compose box in chat with User C

Draft icon continues to show when text does not exist in the compose box

1.Open a chat with User A
2.Enter text into the compose box (but do not send), wait 3-5 seconds
3.Switch to chat with User B
4.Go back to chat with User A, delete the text drafted in compose box
5.Switch to chat with User C
6.Notice: Draft icon continues to show next to User A even though you deleted the text in the compose box

Tested On

Screenshots

Web

screen-recorder-tue-mar-30-2021-22-52-41.mp4

Mobile Web

Mobile.web.mp4

Desktop

screen-recorder-wed-mar-31-2021-23-01-14.mp4

iOS

I don't have a mac to test and record the screen

Android

screen-recorder-thu-apr-01-2021-08-09-54.mp4

github-actions · 2021-03-31T01:35:53Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

0xthierry · 2021-03-31T01:40:18Z

src/pages/home/report/ReportView.js

@@ -30,6 +30,7 @@ class ReportView extends React.Component {
                    <ReportActionCompose
                        onSubmit={text => addAction(this.props.reportID, text)}
                        reportID={this.props.reportID}
+                        key={this.props.reportID}


To help in review:

Basically the TextInputFocusable is an uncontrolled input, so when we change the context (reportID) this component have their own value and don't receive the new value from the switch between reportID.

And the change doesn't occur because we sent the value in the defaultValue property and when we change the context(reportID) this input already have a value, so defaultValue isn't used as a initial value to value property.

The solution is when change the context, we could re-render(with key property) the component(ReportActionCompose) and it will solve this problems.

0xthierry · 2021-03-31T01:44:15Z

I have read the CLA Document and I hereby sign the CLA

0xthierry · 2021-04-01T01:37:26Z

src/pages/home/report/ReportActionCompose.js

@@ -64,25 +64,16 @@ class ReportActionCompose extends React.Component {
        this.comment = props.comment;

        this.state = {
-            isFocused: false,
+            isFocused: true,


When the component is mounted the TextInputFocusable is already focused

0xthierry · 2021-04-01T01:38:40Z

src/pages/home/report/ReportActionCompose.js

            textInputShouldClear: false,
            isCommentEmpty: props.comment.length === 0,
            isMenuVisible: false,
        };
    }

    componentDidUpdate(prevProps) {
-        // The first time the component loads the props is empty and the next time it may contain value.
-        // If it does let's update this.comment so that it matches the defaultValue that we show in textInput.
-        if (this.props.comment && prevProps.comment === '' && prevProps.comment !== this.props.comment) {


The first time the component is mounted this.comment is attributed in the constructor(){...} of this component

0xthierry · 2021-04-01T11:40:30Z

@iwiznia The PR is ready for review

Luke9389 · 2021-04-01T18:05:36Z

Hey @Thierrysantos, thanks for the helpful self-review. I'm you assigned reviewer, and I had seen this yesterday but have asked a few questions internally that I'd like to get answered before I leave my review. Sorry I didn't post here to let you know I was working on it.

Luke9389 · 2021-04-01T18:42:33Z

Nice work! I finished off the testing by doing this on iOS.

Luke9389

Great job! Thanks

OSBotify · 2021-04-01T18:43:34Z

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

iwiznia · 2021-04-01T22:27:57Z

Sorry for not getting to this before, it should've been assigned to me (but I realize now that our instructions are wrong).
Anyway, @Thierrysantos does this fix solve all problems described in the issue? I see you only did one test.

0xthierry · 2021-04-01T22:35:40Z

Yeah, It solves @iwiznia.

In the description of the issue @stephanieelliott show 3 scenarios, I could record this 3 scenarios if you want.

0xthierry · 2021-04-01T22:36:17Z

but I realize now that our instructions are wrong

What is wrong?

0xthierry · 2021-04-01T22:36:22Z

but I realize now that our instructions are wrong

What is wrong?

iwiznia · 2021-04-01T22:38:00Z

Yeah, It solves @iwiznia.
In the description of the issue @stephanieelliott show 3 scenarios, I could record this 3 scenarios if you want.

Oh nice! I don't think we need recording of the 3 scenarios, just listing the tests and including them for QA, so they can verify all is working properly before deploy.

iwiznia · 2021-04-01T22:38:50Z

What is wrong?

Nothing on your part, we are tweaking how the review process is and seems we have not updated our instructions to match it. The idea is that whoever reviews the proposal in the issue, reviews the PR.

0xthierry · 2021-04-01T22:46:32Z

@iwiznia QA Test updated 👍🏾

Nothing on your part, we are tweaking how the review process is and seems we have not updated our instructions to match it. The idea is that whoever reviews the proposal in the issue, reviews the PR.

Great, yeah that make sense

0xthierry · 2021-04-02T19:16:52Z

@iwiznia Do you know if it's already tested by QAs?

iwiznia · 2021-04-02T19:33:51Z

When QA finishes testing all PRs, they get deployed and you should see a comment here saying it was deployed.

0xthierry requested a review from a team as a code owner March 31, 2021 01:35

MelvinBot requested review from Luke9389 and removed request for a team March 31, 2021 01:35

0xthierry commented Mar 31, 2021

View reviewed changes

0xthierry force-pushed the fix/draft-comment branch from 7aa3b6f to a4d2c59 Compare April 1, 2021 01:15

0xthierry mentioned this pull request Apr 1, 2021

Draft message feature not working correctly #2100

Closed

0xthierry marked this pull request as draft April 1, 2021 01:25

fix:synchronization between state in report switch

416b40f

0xthierry force-pushed the fix/draft-comment branch from a4d2c59 to 416b40f Compare April 1, 2021 01:36

0xthierry commented Apr 1, 2021

View reviewed changes

0xthierry marked this pull request as ready for review April 1, 2021 01:39

Luke9389 approved these changes Apr 1, 2021

View reviewed changes

Luke9389 merged commit edf3b3a into Expensify:master Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix:synchronization between state in report switch #2166

fix:synchronization between state in report switch #2166

0xthierry commented Mar 31, 2021 •

edited

Loading

github-actions bot commented Mar 31, 2021 •

edited

Loading

0xthierry Mar 31, 2021

0xthierry commented Mar 31, 2021

0xthierry Apr 1, 2021

0xthierry Apr 1, 2021

0xthierry commented Apr 1, 2021

Luke9389 commented Apr 1, 2021

Luke9389 commented Apr 1, 2021

Luke9389 left a comment

OSBotify commented Apr 1, 2021

iwiznia commented Apr 1, 2021 •

edited

Loading

0xthierry commented Apr 1, 2021

0xthierry commented Apr 1, 2021

0xthierry commented Apr 1, 2021

iwiznia commented Apr 1, 2021

iwiznia commented Apr 1, 2021

0xthierry commented Apr 1, 2021

0xthierry commented Apr 2, 2021

iwiznia commented Apr 2, 2021

fix:synchronization between state in report switch #2166

fix:synchronization between state in report switch #2166

Conversation

0xthierry commented Mar 31, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

github-actions bot commented Mar 31, 2021 • edited Loading

0xthierry Mar 31, 2021

Choose a reason for hiding this comment

0xthierry commented Mar 31, 2021

0xthierry Apr 1, 2021

Choose a reason for hiding this comment

0xthierry Apr 1, 2021

Choose a reason for hiding this comment

0xthierry commented Apr 1, 2021

Luke9389 commented Apr 1, 2021

Luke9389 commented Apr 1, 2021

Luke9389 left a comment

Choose a reason for hiding this comment

OSBotify commented Apr 1, 2021

iwiznia commented Apr 1, 2021 • edited Loading

0xthierry commented Apr 1, 2021

0xthierry commented Apr 1, 2021

0xthierry commented Apr 1, 2021

iwiznia commented Apr 1, 2021

iwiznia commented Apr 1, 2021

0xthierry commented Apr 1, 2021

0xthierry commented Apr 2, 2021

iwiznia commented Apr 2, 2021

0xthierry commented Mar 31, 2021 •

edited

Loading

github-actions bot commented Mar 31, 2021 •

edited

Loading

iwiznia commented Apr 1, 2021 •

edited

Loading