Closed srikarparsi closed 1 week ago
@situchan @ One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]
Yeahh I think we need both the frontend and the backend change right? Because even if the report isn't hidden
, this check will return false without this PR since the chat will be empty.
But the backend change is here. I'm about to step out so wasn't able to fill out everything but I'll do that tonight once I'm back. There's also an issue with Onyx updates I believe because even though we're setting the notificationPreference to hidden
, the response of OpenReport is always
. I don't think that it affects this issue directly but we should still solve it so I'll try to expand more on that and figure out a fix once I'm back.
Yeahh I think we need both the frontend and the backend change right? Because even if the report isn't hidden, this check will return false without this PR since the chat will be empty.
Oh I see - so they are hiding when empty by default. If that's the case then you are right! We need this + the backend change since they will still hide because they are notificationPreferences = 'hidden'
π
LGTM2. Testing shortly
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
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)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
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.Not sure if I am doing wrong but group chat still disappears from LHN when switch report
https://drive.google.com/file/d/11fqzc0pXFCNI8sV4yRNGNb5UbokMFfgi/view?usp=sharinga
Just fixed that. But even before fixing that, I'm not able to reproduce what you were facing @situchan. @marcaaron, do you think you can try creating a new GC with this branch and checking if it stays in the LHN when you navigate away? You don't need to checkout the Auth branch, just this one should be good.
I debugged myself and confirmed @marcaaron is correct.
Newly created group chat is not visible in LHN because "notificationPreference" is set to "hidden".
After latest change, works fine locally but not on another device.
Repro step:
@youssef-lr Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]
After latest change, works fine locally but not on another device.
I'm not able to reproduce this. Am I doing the right steps @situchan?
https://github.com/Expensify/App/assets/48188732/e64f5eae-ae72-412c-a99c-8a89bb0b9165
After latest change, works fine locally but not on another device.
I'm not able to reproduce this. Am I doing the right steps @situchan?
Screen.Recording.2024-05-22.at.2.35.29.AM.mov
yes, right step
:hand: 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/chiragsalian in version: 1.4.76-0 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Just retested with fresh account on staging. Not able to reproduce on mWeb.
π Deployed to production by https://github.com/Beamanator in version: 1.4.76-7 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Details
Fixed Issues
$ https://github.com/Expensify/App/issues/40314 PROPOSAL:
Tests
Offline tests
QA Steps
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
Didn't add because mWeb Chrome covers the smaller screen caseAndroid: mWeb Chrome
iOS: Native
Didn't add because mWeb Safari covers the smaller screen caseiOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop