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.

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

fix: New chat aren't showing bold in Left Hand Nav #6898

Merged
merged 2 commits into from
Dec 27, 2021

Conversation

aswin-s
Copy link
Contributor

@aswin-s aswin-s commented Dec 23, 2021

Details

This PR fixes an issue where new chat messages were not being shown in bold inside LHN.

Fixed Issues

$ #6874

Tests

  1. Sign in as User A in one browser tab & as User B in another tab
  2. Open chat with the other user in each tab.
  3. Now navigate to a separate conversation in LHN for User B. (For example Concierge)
  4. Send a message from User A to User B.
  5. The new message should be highlighted in bold on the LHN for User B.
  6. Verify that it works other way around as well.

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

fix_unread.mp4

Mobile Web

N.A

Desktop

image

iOS

image

Android

Screenshot_1640299864

@aswin-s aswin-s requested a review from a team as a code owner December 23, 2021 22:51
@MelvinBot MelvinBot requested review from marcochavezf and parasharrajat and removed request for a team December 23, 2021 22:51
Copy link
Contributor

@marcochavezf marcochavezf left a comment

Choose a reason for hiding this comment

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

Looks good to me! cc @parasharrajat

@marcochavezf marcochavezf merged commit 22a7baf into Expensify:main Dec 27, 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.

@marcochavezf
Copy link
Contributor

marcochavezf commented Dec 27, 2021

Merging it and applying the CP Staging label because more employees are going to experience this issue when are back from holidays.

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

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.

Thanks for the changes @aswin-s and waiting for review I was on vacation. This looks good.

Could you please add videos for all platforms and present the following features;

  1. The current issue.
  2. Unread Message when the app is put to foreground from background.

@@ -134,7 +135,8 @@ class Expensify extends PureComponent {
}

componentWillUnmount() {
AppState.removeEventListener('change', this.initializeClient);
if (!this.appStateChangeListener) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

Could you please expand this to multiple lines?

Suggested change
if (!this.appStateChangeListener) { return; }
if (!this.appStateChangeListener) {
return;
}

@@ -85,6 +85,7 @@ class Expensify extends PureComponent {
ActiveClientManager.init();
this.setNavigationReady = this.setNavigationReady.bind(this);
this.initializeClient = this.initializeClient.bind(true);
this.appStateChangeListener = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.appStateChangeListener = null;

@@ -106,6 +106,7 @@ class ReportActionsView extends React.Component {
this.recordTimeToMeasureItemLayout = this.recordTimeToMeasureItemLayout.bind(this);
this.loadMoreChats = this.loadMoreChats.bind(this);
this.sortedReportActions = [];
this.appStateChangeListener = null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.appStateChangeListener = null;

@aswin-s aswin-s deleted the aswin-s/issue_6874 branch December 27, 2021 18:45
@aswin-s aswin-s restored the aswin-s/issue_6874 branch December 27, 2021 18:51
@aswin-s
Copy link
Contributor Author

aswin-s commented Dec 27, 2021

@parasharrajat Should I raise a follow up PR with the suggested changes?

@parasharrajat
Copy link
Member

These changes are no blocker for me so If @marcochavezf says we should do these then yeah please add a follow-up?

@marcochavezf
Copy link
Contributor

These changes are no blocker for me so If @marcochavezf says we should do these then yeah please add a follow-up?

@parasharrajat Yeah, I think we could include these changes as a follow-up, thank you for doing it @aswin-s

@roryabraham
Copy link
Contributor

roryabraham commented Dec 28, 2021

Hi, I'm now seeing the following error on main:

@roryabraham
Copy link
Contributor

It looks like this change was made in react-native 0.65, but we're still using react-native 0.64.1

@roryabraham
Copy link
Contributor

I think we need to either revert this PR or upgrade the whole app to RN 0.65

@aswin-s
Copy link
Contributor Author

aswin-s commented Dec 28, 2021

@roryabraham as explained over here, removeEventListener was not working on react-native-web. I guess upgrading to 0.65 will solve it for both platforms, considering there is only a small diff between both versions.

https://react-native-community.github.io/upgrade-helper/?from=0.64.1&to=0.65.0

Related to this, I would also like to point out that dependencies like react-native-modal is also consistently throwing similar error on web which is fixed in their latest release. Unfortunately that dependency upgrade is also blocked by react-native peer dependency requirement of ">=0.65.0"

WDYT?

@roryabraham
Copy link
Contributor

roryabraham commented Dec 28, 2021

I suppose we should update react-native and the other dependencies that are throwing the error. I'll work on a pull request to upgrade RN.

@roryabraham
Copy link
Contributor

I have an upgrade PR here, but I could use help testing it. Works well on iOS from my testing so far.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcochavezf in version: 1.1.23-2 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Jan 4, 2022

🚀 Deployed to production by @francoisl in version: 1.1.24-8 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants