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

Fix avatar online status icon always show sync icon #7977

Merged
merged 3 commits into from
Mar 14, 2022
Merged

Fix avatar online status icon always show sync icon #7977

merged 3 commits into from
Mar 14, 2022

Conversation

K4tsuki
Copy link
Contributor

@K4tsuki K4tsuki commented Mar 2, 2022 •
edited by flodnv
Loading

Details

Fixed Issues

$ #7730

Tests

  • Verify that no errors appear in the JS console
    In web:
  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, close browser tab (shortcut in firefox ctrl + w)
  4. Open the expensify again
  5. Verify online icon doesn't display sync icon, but green / online icon

In Desktop:

  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, quit the App by pressing shortcut Command (⌘)-Q
  4. Open the expensify again in browser
  5. Verify online icon doesn't display sync icon, but green / online icon

In Android/IOS:

  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, force close the application (not backgrounding it)
  4. Open the expensify application again
  5. Verify online icon doesn't display sync icon, but green / online icon

PR Review Checklist

Contributor (PR Author) Checklist

  • I made sure to pull main before submitting my PR for review
  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I clearly indicated the environment tests should be run in (Staging vs Production)
  • I wrote testing steps that cover success & fail scenarios (if applicable)
  • I ran the tests & they passed on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I followed proper code patterns (see Reviewing the code)
    • I added comments when the code was not self explanatory
    • I put all copy / text shown in the product in all src/languages/* files (if applicable)
    • I followed proper naming convention for platform-specific files (if applicable)
    • I followed style guidelines (in Styling.md) for all style edits I made
  • I followed the guidelines as stated in the Review Guidelines

PR Reviewer Checklist

  • I verified the Author pulled main before submitting the PR
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I verified tests pass on all platforms & I tested again on all platforms
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines

QA Steps

  • Verify that no errors appear in the JS console
    In web:
  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, close browser tab (shortcut in firefox ctrl + w)
  4. Open the expensify again
  5. Verify online icon doesn't display sync icon, but green / online icon

In Desktop:

  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, quit the App by pressing shortcut Command (⌘)-Q
  4. Open the expensify again in browser
  5. Verify online icon doesn't display sync icon, but green / online icon

In Android:

  1. Open App
  2. Turn off and turn on wifi/internet
  3. when sync icon appear and before disappear in avatar online status, force close the application (not backgrounding it)
  4. Open the expensify application again
  5. Verify online icon doesn't display sync icon, but green / online icon

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Before:

sync_icon_web_before.mp4

After:

sync_icon_web_after.mp4

Mobile Web

mobile_web_fx.mp4

Desktop

deskto_sync_icon.mp4

iOS

Ios_sync_icon.mp4

Android

android.mp4

@K4tsuki K4tsuki requested a review from a team as a code owner March 2, 2022 07:43
@MelvinBot MelvinBot requested review from flodnv and parasharrajat and removed request for a team March 2, 2022 07:43
parasharrajat
parasharrajat suggested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Changes look good. but screenshots for all platforms must be added to the PR. I can make an exception for this PR but you will need it for other PRs so I suggest it is a good time to set up Mac.

parasharrajat
parasharrajat suggested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Also, you will have to sign all commits before this PR can be merged.

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Mar 5, 2022 •
edited
Loading

@parasharrajat I have sign all commits.

Also In different slack thread :
https://app.slack.com/client/T03SC9DTT/threads/thread/C01GTK53T8Q-1644857187.920329

Issue reported by @adeel0202 is detected as same issue with this issue, but I think it is different issue. Should I submit a commit to fix the issue too?

@parasharrajat
Copy link
Member

Let's wait for the QA team to mention that on GH first.

parasharrajat
parasharrajat suggested changes Mar 8, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Code 👍

@K4tsuki Please do the following:

  1. Add a video for Desktop.
  2. Then tick the checkboxes on the checklist.
  3. Please add more descriptive QA steps. Mention the platform in the tests. Write down steps that are different on each platform.

Thanks.

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Mar 12, 2022

@parasharrajat I have attached desktop video, update QA steps and add ticks on checkbox list.

@parasharrajat
Copy link
Member

Sorry, I just saw that iOS is also missing. Please add that as well.

@K4tsuki
Copy link
Contributor Author

K4tsuki commented Mar 12, 2022

@parasharrajat I have attached IOS video.

parasharrajat
parasharrajat approved these changes Mar 12, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

LGTM,

cc: @flodnv

PR Reviewer Checklist

  • I verified the PR has a small number of commits behind main
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the testing environment is mentioned in the test steps
  • I verified testing steps cover success & fail scenarios (if applicable)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors related to changes in this PR
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified comments were added when the code was not self explanatory
    • I verified any copy / text shown in the product was added in all src/languages/* files (if applicable)
    • I verified proper naming convention for platform-specific files was followed (if applicable)
    • I verified style guidelines were followed
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified the UI performance was not affected (the performance is the same than main branch)
  • If a new component is created I verified that a similar component doesn't exist in the codebase

🎀 👀 🎀 C+ reviewed

@flodnv flodnv merged commit 0415470 into Expensify:main Mar 14, 2022
@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 OSBotify mentioned this pull request Mar 15, 2022
Closed
52 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @flodnv in version: 1.1.43-0 🚀

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

@botify botify mentioned this pull request Mar 17, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.43-2 🚀

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

@SumitDiyora SumitDiyora mentioned this pull request Apr 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat approved these changes

@flodnv flodnv flodnv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@K4tsuki @parasharrajat @OSBotify @flodnv