Desktop

andrewlor · 2022-09-20T19:16:38Z

Details

On the desktop app, minimizing and restoring the app does not change the visibility state. As a result the new message marker is not disappearing as expected in this case. So this PR is modifying the behaviour of isVisible for the desktop app build so we can achieve the desired behaviour in this case.

Fixed Issues

$ #10767

Tests

Run the desktop app
Open a chat with an unread message
Minimize and Restore the app window
Verify the new message marker has disappeared from the chat

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Run the desktop app
Open a chat with an unread message
Minimize and Restore the app window
Verify the new message marker has disappeared from the chat

Verify that no errors appear in the JS console

Screenshots

Web

chrome.mov

safari.mov

firefox.mov

Mobile Web

safari-ios.mov

desktop.mov

iOS

ios.mov

Android

android.mp4

thienlnam

Looks good, just small note about the comment

src/libs/Visibility/index.js

…ActionsView

marcaaron

Few comments - mostly related to the HOC usage. I think if we can get the electron listener stuff into a Visibility.onFocus() listener we can simplify things a bit.

src/components/withWindowVisibility/index.desktop.js

src/pages/home/report/ReportActionsView.js

src/libs/Visibility/index.js

rushatgabhane

@andrewlor I like the new changes!

@thienlnam LGTM and tests well! 🎉

PR Reviewer Checklist

thienlnam

Changes are looking solid 👍 Agree with the comment above about making it more about subscribing or adding an event listener

src/libs/Visibility/index.js

…ener

thienlnam · 2022-09-30T17:05:20Z

cc @marcaaron Could you please take another look?

desktop/contextBridge.js

src/libs/Visibility/index.desktop.js

rushatgabhane · 2022-10-07T13:34:41Z

@thienlnam oops, looks like we missed this PR (because we weren't re-requested for a review)

Let's get it merged if all looks good to you as well?

thienlnam · 2022-10-11T12:41:31Z

src/libs/Visibility/index.native.js

+function onVisibilityChange(callback) {
+    // Deliberately strip callback argument to be consistent across implementations
+    const subscription = AppState.addEventListener('change', () => callback());
+


NAB: remove this extra line

thienlnam · 2022-10-11T12:41:37Z

src/libs/Visibility/index.js

+function onVisibilityChange(callback) {
+    // Deliberately strip callback argument to be consistent across implementations
+    const subscription = AppState.addEventListener('change', () => callback());
+


NAB: remove this extra line

thienlnam · 2022-10-11T12:41:53Z

src/libs/Visibility/index.native.js

+ *
+ * @param {Function} callback
+ *
+ * @return {Function} removes the listener


NAB: remove extra lines

marcaaron · 2022-10-11T20:08:32Z

@rushatgabhane can you try to see why the checklist step is failing here? I think maybe there have been some changes to the one previously posted.

rushatgabhane · 2022-10-11T20:23:01Z

@marcaaron yep, check for displayName was removed.
Checks are passing now

marcaaron · 2022-10-12T22:18:03Z

Changes look good here, nice work all around.

But, looks like the checklist step is failing so probably something about it has changed and will need to be updated.

@rushatgabhane and we just need videos for all platforms to confirm this is working as intended and then can proceed with the merge.

Thanks everyone!

andrewlor · 2022-10-17T16:49:33Z

Updated screen recordings on all platforms (updated videos in PR description as well)

chrome.mov

safari.mov

firefox.mov

safari-ios.mov

desktop.mov

ios.mov

android.mp4

rushatgabhane · 2022-10-17T18:13:52Z

Web

Screen.Recording.2022-10-17.at.10.59.28.PM.mov

mWeb

Screen.Recording.2022-10-17.at.11.03.31.PM.mov

Screen.Recording.2022-10-17.at.11.41.35.PM.mov

iOS

Screen.Recording.2022-10-17.at.11.37.39.PM.mov

Android

WhatsApp.Video.2022-10-17.at.23.31.32.mp4

Desktop

Screen.Recording.2022-10-17.at.10.56.21.PM.mov

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
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 steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos 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
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

rushatgabhane · 2022-10-17T18:20:36Z

Interesting that the PR review checklist is failing. I can't find the diff 🥲

melvin-bot · 2022-10-17T23:17:38Z

@thienlnam looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

thienlnam · 2022-10-17T23:18:34Z

Just merged it, I think it might be possible that the reviewer checklist was too far up and was not in the API results? 🤷

OSBotify · 2022-10-17T23:22:26Z

✋ 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 · 2022-10-19T12:22:57Z

🚀 Deployed to staging by @thienlnam in version: 1.2.18-0 🚀

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

OSBotify · 2022-10-19T13:45:59Z

🚀 Deployed to staging by @thienlnam in version: 1.2.18-0 🚀

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

OSBotify · 2022-10-24T23:34:04Z

🚀 Deployed to production by @chiragsalian in version: 1.2.18-10 🚀

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

fix-issue-10767 modify isVisible behaviour

5f04fba

andrewlor requested a review from a team as a code owner September 20, 2022 19:16

melvin-bot bot requested review from rushatgabhane and thienlnam and removed request for a team September 20, 2022 19:16

thienlnam suggested changes Sep 20, 2022

View reviewed changes

src/libs/Visibility/index.js Outdated Show resolved Hide resolved

thienlnam mentioned this pull request Sep 21, 2022

[HOLD for payment 2022-10-31] [Bug] [$250] Desktop - Chat- New marker doesn't disappear after App minimize/restore #10767

Closed

fix-issue-10767 seperate window visibility implementations for Report…

171af45

…ActionsView

marcaaron requested changes Sep 22, 2022

View reviewed changes

src/components/withWindowVisibility/index.desktop.js Outdated Show resolved Hide resolved

src/components/withWindowVisibility/index.desktop.js Outdated Show resolved Hide resolved

src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved

fix-issue-10767 remove HOC and use listener approach

e8b784e

rushatgabhane reviewed Sep 26, 2022

View reviewed changes

src/libs/Visibility/index.js Outdated Show resolved Hide resolved

rushatgabhane previously approved these changes Sep 26, 2022

View reviewed changes

thienlnam reviewed Sep 26, 2022

View reviewed changes

src/libs/Visibility/index.js Outdated Show resolved Hide resolved

fix-issue-10767 rename Visibility.onChange to Visibility.addEventList…

74262d5

…ener

andrewlor dismissed rushatgabhane’s stale review via 74262d5 September 26, 2022 18:27

rushatgabhane previously approved these changes Sep 26, 2022

View reviewed changes

thienlnam previously approved these changes Sep 26, 2022

View reviewed changes

andrewlor requested a review from marcaaron September 26, 2022 21:59

marcaaron assigned andrewlor Sep 30, 2022

marcaaron requested changes Sep 30, 2022

View reviewed changes

fix-issue-10767 PR comments

5f27df5

andrewlor dismissed stale reviews from thienlnam and rushatgabhane via 5f27df5 September 30, 2022 22:37

fix-issue-10767 fix

5f11cff

rushatgabhane approved these changes Oct 7, 2022

View reviewed changes

andrewlor requested review from marcaaron and thienlnam and removed request for marcaaron October 7, 2022 14:47

thienlnam approved these changes Oct 11, 2022

View reviewed changes

andrewlor requested a review from marcaaron October 11, 2022 16:54

This comment was marked as outdated.

Sign in to view

marcaaron approved these changes Oct 12, 2022

View reviewed changes

thienlnam merged commit 4dc7fef into Expensify:main Oct 17, 2022

melvin-bot bot added the Emergency label Oct 17, 2022

thienlnam removed the Emergency label Oct 17, 2022

OSBotify mentioned this pull request Oct 19, 2022

Deploy Checklist: New Expensify 2022-10-19 #11979

Closed

36 tasks

Conversation

andrewlor commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

thienlnam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushatgabhane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

PR Reviewer Checklist

Uh oh!

thienlnam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thienlnam commented Sep 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushatgabhane commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thienlnam Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

thienlnam Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

thienlnam Oct 11, 2022

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Oct 11, 2022

Uh oh!

This comment was marked as outdated.

rushatgabhane commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Oct 12, 2022

Uh oh!

andrewlor commented Oct 17, 2022

Uh oh!

rushatgabhane commented Oct 17, 2022

Web

mWeb

iOS

Android

Desktop

PR Reviewer Checklist

Uh oh!

rushatgabhane commented Oct 17, 2022

Uh oh!

melvin-bot bot commented Oct 17, 2022

Uh oh!

thienlnam commented Oct 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

andrewlor commented Sep 20, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Oct 7, 2022 •

edited

Loading

rushatgabhane commented Oct 11, 2022 •

edited

Loading

thienlnam commented Oct 17, 2022 •

edited

Loading