Closed janicduplessis closed 4 days ago
@jayeshmangwani 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]
@perunt Would you be able to confirm this change is ok since you wrote that code initially?
I also noticed on iOS there seems to be an issue where newer messages don't load initially unless you scroll back up a little, you can see in the video in the screenshots section. I will investigate that in a follow up.
@janicduplessis Is this a follow-up PR of any other PR?
I found the issue while working on https://github.com/Expensify/App/pull/40610, but it is not directly related.
@rojiphil This PR is somewhat related to this PR https://github.com/Expensify/App/pull/40610, Would you complete the checklist here, or should I start the review?
I traced it back to this commit which refactored the logic to a variable, but incorrectly.
@janicduplessis From the PR description it looks like the root cause lies elsewhere and is unrelated to PR #40610.
Would you complete the checklist here, or should I start the review?
@jayeshmangwani Please go ahead if you are available here. I am not sure if I can get to this in the next 2-3 days.
@jayeshmangwani Please go ahead with this PR
On it...
@janicduplessis Is there any way to test the Android loader? I have tried with the Steps in the Tests and deep link, but not able to see the Loader for Android native
https://github.com/Expensify/App/assets/35371050/f65307d6-b2ec-4893-a6d6-0a25ab091151
Is there any way to test the Android loader? I have tried with the Steps in the Tests and deep link, but not able to see the Loader for Android native
@janicduplessis
Running a build @janicduplessis are you able to help @jayeshmangwani please?
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/42332/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/42332/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/42332/NewExpensify.dmg | https://42332.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
@jayeshmangwani The seems to be another issue on mobile where the newer messages are not loading unless you scroll up some. Does it work if you try to scroll up for about 1-2 screens of height then scroll back down?
I will try to look at that issue next.
Does it work if you try to scroll up for about 1-2 screens of height then scroll back down?
Now, After trying several times, I am able to show the loader a few times
### 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.Code changes looks good, and Tests Well.
NAB: Videos are missing for some platforms
: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/mountiny in version: 1.4.77-0 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
platform | result |
---|---|
🤖 android 🤖 | failure ❌ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.77-11 🚀
platform | result |
---|---|
🤖 android 🤖 | success ✅ |
🖥 desktop 🖥 | success ✅ |
🍎 iOS 🍎 | success ✅ |
🕸 web 🕸 | success ✅ |
Details
I noticed that the loading indicator for loading new messages is not displayed.
I traced it back to this commit which refactored the logic to a variable, but incorrectly.
It changed
to
The correct change is:
When testing on iOS I also noticed the scrollingVerticalOffset check was preventing the loading indicator from showing some times. I don't see why it would be needed and didn't seem to introduce any problem when testing loading other chats.
After this the header now shows correctly on 2nd render as the code was meant to do.
Before
https://github.com/Expensify/App/assets/2677334/e144e1f0-bc14-420e-a267-68234b468ed5
After
https://github.com/Expensify/App/assets/2677334/b704385e-891a-4896-a45f-427b417c2da4
Fixed Issues
$ https://github.com/Expensify/App/issues/42331 PROPOSAL:
Tests
Offline tests
QA Steps
Steps to test comment linking:
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
Android: mWeb Chrome
iOS: Native
https://github.com/Expensify/App/assets/2677334/1e10e14c-bbee-4d35-b9f6-c42a6b5b16cdiOS: mWeb Safari
MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/2677334/b704385e-891a-4896-a45f-427b417c2da4MacOS: Desktop