-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fixing duplicate Concierge chat #51794
Conversation
Addressing the issue of duplicate Concierge chats appearing in the Left Hand Navigation (LHN) when deep linking to Concierge.
All contributors have signed the CLA ✍️ ✅ |
@getusha can you review this one today, please? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-04.at.7.50.46.in.the.evening.movAndroid: mWeb ChromeScreen.Recording.2024-11-04.at.7.46.40.in.the.evening.moviOS: NativeScreen.Recording.2024-11-04.at.8.04.38.in.the.evening.moviOS: mWeb SafariScreen.Recording.2024-11-04.at.7.43.13.in.the.evening.movMacOS: Chrome / SafariScreen.Recording.2024-11-04.at.7.33.39.in.the.evening.movMacOS: DesktopN/a |
this should be https://expensify.com/ Any luck on the android build? and you need to sign CLA just follow this #51794 (comment) |
I have read the CLA Document and I hereby sign the CLA |
recheck |
@getusha I was able to build the android app after pulling from the main branch. I uploaded the video. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ugogiordano there are some linter checks failing, can you fix them please to continue with this 🙏
fixing linter checks
Hi @aldo-expensify, Sorry for the late reply—I didn't notice the failing checks earlier. Thanks for pointing it out! |
There is a check failing because of the existing
Can you replace that with @getusha friendly bump for reviewing this |
@aldo-expensify, I see that I can work on it but it will require a little bit of refactoring. |
No, no need to refactor other files. I think if you just fix it in the file you touched the checks should pass. The check is complaining only about |
That's clear, but I'm only referring to the
I'm testing again all the platform before updating the PR. I was wondering if we could take another look at the compensation for this job. |
Migrating from `withOnyx` to `useOnyx`.
In my opinion the change is too small to consider extra compensation and having to retest isn't reason enough either. I asked for a second opinion in https://expensify.slack.com/archives/C03TQ48KC/p1732059846984899, and there was agreement on not being enough to consider extra compensation. If you still think you should have extra compensation feel free to bring it up in Thanks for committing the update to use @getusha please review when you have some time 🙇 |
No problem. I just saw other issues asking the same small refinement, and that’s why I asked. Can I ask you how I can join that slack channel? I’ve submitted the required form several times with no luck. |
@ugogiordano let's update the tests step
|
@getusha done! |
@ugogiordano thanks! the QA steps as well and we don't need offline tests |
@getusha done! |
Seems like we are having issues for the moment with adding new users to the slack channel:
is this the form you already filled? |
Yes that's the form I've submitted a couple of times. |
Ahh, I think this is an ongoing issue and probably won't help to submit it more times. Sorry about that! You will just have to wait until a solution is found on our end. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.0.65-1 🚀
|
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to production by https://github.com/chiragsalian in version: 9.0.65-5 🚀
|
Resolving the issue of duplicate concierge chat appearing in the LHN when using deeplinks to access the concierge.
Details
Fixed Issues
$ #44465
PROPOSAL: #44465 (comment)
Tests
Here is a video demonstrating the steps to reproduce the issue.
QA Steps
Here is a video demonstrating the steps to reproduce the issue.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-11-05.at.00.07.41.mp4
Android: mWeb Chrome
Screen.Recording.2024-10-31.at.14.43.44.mp4
iOS: Native
Screen.Recording.2024-10-31.at.00.01.11.mp4
iOS: mWeb Safari
Screen.Recording.2024-10-30.at.23.38.34.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-10-30.at.23.28.38.23.41.30.mp4
MacOS: Desktop