Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed offline messages are grayed out even if online #2526

Merged
merged 5 commits into from
Apr 23, 2021
Merged

fixed offline messages are grayed out even if online #2526

merged 5 commits into from
Apr 23, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Apr 22, 2021 •
edited
Loading

Please review @marcaaron

Details

Working on the change request here #2432 (comment)

Fixed Issues

Fixes #2432

Tests / ### QA Steps

  1. Try to send messages in any chat and observe the gray text until they are uploaded to the server.
  2. Put on airplane mode, and send few messages, observe the text appearances.
  3. You should also see a status You appear to be Offline under the composer.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

video.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner April 22, 2021 02:11
@MelvinBot MelvinBot requested review from deetergp and removed request for a team April 22, 2021 02:12
@parasharrajat
Copy link
Member Author

parasharrajat commented Apr 22, 2021 •
edited
Loading

I also see that we are attaching a listener for personaldetails to ReportActionItemSingle. There would a lot of instances of this component on a single screen. Thus I suggest moving the listener up the ReportActionsView like I did here for network.

let me know, If i can push these changes as well.

@marcaaron
Copy link
Contributor

I also see that we are attaching a listener for personaldetails to ReportActionItemSingle. There would a lot of instances of this component on a single screen.

With stuff like this I think if you can time/benchmark and prove there is a perceptible difference then we should do it. But otherwise we could be making unnecessary optimizations.

@parasharrajat
Copy link
Member Author

Updated. Thanks.

@marcaaron marcaaron self-requested a review April 23, 2021 18:08
marcaaron
marcaaron requested changes Apr 23, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See additional comment thanks :)

@deetergp
Copy link
Contributor

TIL: You can toggle online/offline in the browser debugger.

@parasharrajat
Copy link
Member Author

Updated. Thanks.

marcaaron
marcaaron requested changes Apr 23, 2021
View reviewed changes
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better just one more comment about the original implementation here.

src/pages/home/report/ReportActionItemMessage.js Outdated
// reportActionID is only present when the action is saved onto server.
const isUnsent = action.loading && !action.reportActionID;
const isUnsent = network.isOffline && action.loading && !action.reportActionID;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need to check both action.loading and action.reportActionID? What does having a reportActionID even mean?

Copy link
Contributor

@marcaaron marcaaron Apr 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or asked another way are there comments that are "loading" and "have a reportActionID"...? it's unclear how we end up in that situation to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this to make a difference from the attachment files. As they are also in loading status until they are uploaded. action.reportActionID is only present when comment is uploaded to backend.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it makes sense. we don't actually need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they are also in loading status until they are uploaded

Ok, yeah I believe there's no difference there. It should be the same whether attachment or not.

@parasharrajat
Copy link
Member Author

@marcaaron Updated. Thanks. Sorry for the back and forth.

deetergp
deetergp approved these changes Apr 23, 2021
View reviewed changes
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@marcaaron marcaaron merged commit f043f8a into Expensify:main Apr 23, 2021
@OSBotify
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

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

@parasharrajat parasharrajat mentioned this pull request Jan 17, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcaaron marcaaron marcaaron approved these changes

@deetergp deetergp deetergp approved these changes

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@parasharrajat @marcaaron @deetergp @OSBotify