-
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
Remove the categories beta #28296
Remove the categories beta #28296
Conversation
Test fail is not unique to this PR: https://expensify.slack.com/archives/C01GTK53T8Q/p1695788055569909 |
Reviewer Checklist
Screenshots/VideosAndroidCleanShot.2023-09-27.at.18.03.49.mp4 |
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.
LGTM
Should merge main to resolve failing test. |
Conflicts! |
@puneetlath Waiting on your final review! |
Going to approve/merge after this: https://expensify.slack.com/archives/C9YU7BX5M/p1696223764861079 |
It is taking a while so we have some conflicts! |
@puneetlath Do you want me to solve the conflicts since Yuwen is out? |
We should update this line as well : App/src/pages/EditRequestPage.js Line 126 in a63f853
|
@puneetlath friendly bump so we know how to proceed! |
Whoops, sorry, I commented on the GH issue but not here. Just waiting for this bug to be fixed: #28964 Then I'll fix conflicts and you can re-review. Cool? |
@puneetlath Sounds good! I will be ready for another re-review hehe |
Just merged the PR that we were waiting on. So as long as it makes it to production without getting reverted, I'll clean this PR up in the next day or two. |
@pecanoro this is off hold and ready for another quick review! |
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.
Looking good and it seems to work from what I tested!
✋ 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/pecanoro in version: 1.3.92-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.92-4 🚀
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 1.3.93-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀
|
@puneetlath please review (and merge when you feel comfortable to set categories out of beta)
Details
Removes the categories Beta. I.e., shows the categories selectors in the MoneyRequestConfirmationList and the MoneyRequestView
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/308355
Tests
Offline tests
QA Steps
Same as above, both offline and online.
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 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 usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android