Closed bernhardoj closed 3 days ago
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!
@abdulrahuman5196 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]
@youssef-lr I noticed another issue. The cents amount doesn't show immediately as you can see from the video below (0:10)
https://github.com/Expensify/App/assets/50919443/c7793744-d8c2-4079-a64d-19660839c920
This is because the initial state of the amount is always converted to the FE amount that removes the cent (/ 100).
I want to do it like this
but I think formatAmountOnBlur doesn't sound correct. Should we rename it to something else or just keep the name?
Ok I fixed it by moving the logic to a prop (onFormatAmount), this allows for more reusablity for having different formatting logic.
I noticed another issue. The cents amount doesn't show immediately as you can see from the video below
I don't remember seeing this issue initially, could it be from the refactor to using UserListItem
?
bump @abdulrahuman5196 for review please
Checking now
@bernhardoj I am seeing layout issues in android chrome which is not present in staging. Other platforms there is no issue. Could you check on this?
https://github.com/Expensify/App/assets/46707890/931e04c2-1e29-41e7-ba71-84db099f700c
IT's a known issue. I have merged with main to pull the fix.
IT's a known issue. I have merged with main to pull the fix.
Got it. Thank you.
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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.: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/youssef-lr in version: 1.4.77-11 π
platform | result |
---|---|
π€ android π€ | failure β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
π Deployed to staging by https://github.com/youssef-lr in version: 1.4.77-11 π
platform | result |
---|---|
π€ android π€ | success β |
π₯ desktop π₯ | success β |
π iOS π | success β |
πΈ web πΈ | success β |
Details
The input currently use autoGrow which needs to initialize the width from the onLayout event, so for a brief moment, the width is 0. This PR changes it to a fixed width input.
Fixed Issues
$ https://github.com/Expensify/App/issues/41751 PROPOSAL: https://github.com/Expensify/App/issues/41751#issuecomment-2099742434
Tests
Same as QA Steps
Offline tests
Same as QA Steps
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
https://github.com/Expensify/App/assets/50919443/79ef7c28-ae35-4331-a5e3-f5d0e5d9155dAndroid: mWeb Chrome
https://github.com/Expensify/App/assets/50919443/cda81540-1f58-440f-9843-ea7aa2a818b0iOS: Native
https://github.com/Expensify/App/assets/50919443/41039adc-5708-4614-8139-fb7805c9f1d7iOS: mWeb Safari
https://github.com/Expensify/App/assets/50919443/fc250cff-5e8f-4dd6-aaf1-793d7c9a9f59MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/50919443/bfabcd6c-6cf0-43bc-bb03-4e8499f49aacMacOS: Desktop
https://github.com/Expensify/App/assets/50919443/da16da90-7e40-40c3-ba98-18b9d03e017d