Open Krishna2323 opened 2 weeks ago
@DylanDylann 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]
@Krishna2323 This is ready for review, right? I see we are still discussing with design team in the GH issue
Going to spin up a test build for us.
@DylanDylann, removed the container padding of 1px
, you can review.
Screenshots and code changes look good to me though, love how much we simplified this with flex!
: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/41647/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/41647/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/41647/NewExpensify.dmg | https://41647.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
Nice, tests well and looks great!
### 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.@DylanDylann, PR Reviewer Checklist
is failing, can you pls check.
@Krishna2323 please resolve conflicts
@Krishna2323 Could you resolve conflict?
@tylerkaraszewski Please take a look at this PR
Conflicts resolved.
Details
Fixed Issues
$ https://github.com/Expensify/App/issues/41048 PROPOSAL: https://github.com/Expensify/App/issues/41048#issuecomment-2078498139
Tests
For small width devices
[x] Verify that no errors appear in the JS console
Offline tests
QA Steps
For small width devices
[x] Verify that no errors appear in the JS console
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
https://github.com/Expensify/App/assets/85894871/e4a520e1-5ab0-497c-aac0-d3f98dc4afceAndroid: mWeb Chrome
https://github.com/Expensify/App/assets/85894871/d564cd3a-28fb-4f8f-8dcf-75e502d912c6iOS: Native
https://github.com/Expensify/App/assets/85894871/7223efe7-73df-46d0-817f-305e3c958c63iOS: mWeb Safari
https://github.com/Expensify/App/assets/85894871/e235d578-4a76-4ae1-81fa-ad2a0e04a483MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/85894871/3bce63ce-2b86-4ed8-99f5-51b02fa07f64MacOS: Desktop
https://github.com/Expensify/App/assets/85894871/271db6c3-ad64-4956-8fb6-ffcff8abc96b