-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: load policy categories only when needed #49756
feat: load policy categories only when needed #49756
Conversation
@dukenv0307 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] |
For offline mode after the user selects a workspace, since no categories are loaded for workspace it will show like below instead. This seems incorrect as the workspace does have categories but they are not loaded because user is offline. Should we just show a message that says you need to be online to view this or do we have a common component for this? |
src/components/CategoryPicker.tsx
Outdated
const [policyCategoriesDraft] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_CATEGORIES_DRAFT}${policyID}`); | ||
const [policyRecentlyUsedCategories] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_RECENTLY_USED_CATEGORIES}${policyID}`); | ||
|
||
Category.getPolicyCategories(policyID); |
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.
we shouldn't call API in here. You can do the same as openPolicyCategoriesPage
@jaydamani I think we can show the loading skeleton or OfflineIndicator page cc @shawnborton |
There might be an oversight. There is already an API call which loads the categories when user selects the policy so, the new endpoint may not be needed. |
@jaydamani What do you mean? |
@dukenv0307 As part of the solution for the orignal issue, we created a new endpoint getPolicyCategories from backend. This PR updates frontend to use the new endpoint for the "categorise it" flow to load all categories after the user selects a workspace but while making the changes from review I noticed that "categorise it" flow already calls another endpoint from here which loads required data related to workspace including the categories for that workspace. So we may not need new endpoint to load categories from the CategoryPicker. |
The new endpoint is the same as openDraftWorkspaceRequest, it's an alias so the request is 1:1:1 for this different use-case. Also, I agree we will need to update this offline pattern since you cannot categorize offline so probably the OfflineIndicator |
@dukenv0307 This is now ready for review. |
@jaydamani Thank you. I'm reviewing... |
@jaydamani I think we shouldn't use we can do the same as |
@dukenv0307 I also agree that looks more appropriate. should we keep it consistent and block the whole flow so show full page offline view when user clicks on "categorize it" or only show it when user selects a workspace and we do not have data for that workspace.
|
Here is my thought:
|
I think this approach makes sense to me. cc @trjExpensify @dubielzyk-expensify for thoughts as well. |
That makes sense to me! |
Yep that looks good to me |
@@ -25,6 +25,9 @@ type StepScreenWrapperProps = { | |||
/** Whether or not to display not found page */ | |||
shouldShowNotFoundPage?: boolean; | |||
|
|||
/** Whether to show offline indicator */ | |||
shouldShowOfflineIndicator?: boolean; |
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.
Can you explain why this change?
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.
StepScreenWrapper renders ScreenWrapper which shows OfflineInidicator at bottom for small screen but our page would already show that user is offline so showing an indicator at bottom would feel weird.
App/src/components/ScreenWrapper.tsx
Line 123 in 9627f5f
shouldShowOfflineIndicator = true, |
App/src/components/ScreenWrapper.tsx
Line 288 in 9627f5f
{isSmallScreenWidth && shouldShowOfflineIndicator && <OfflineIndicator style={offlineIndicatorStyle} />} |
Another approach I tried was to add below code to IOURequestStepCategory but then the header will not render because it comes from StepScreenWrapper so the current approach seemed better
if (policyCategories === undefined && isOffline) {
// a children is mandatory for FullPageOfflineBlockingView
return <FullPageOfflineBlockingView>{null}</FullPageOfflineBlockingView>
}
@jaydamani In case we already loaded policy categories, but all of them are disabled or it's empty, should we show IMO, we should show |
Here's the current behavior: Screen.Recording.2024-10-03.at.10.49.22.mov |
@jaydamani Can you take a look at my concern above? |
I was away yesterday, will work on it and push today. |
@jaydamani Is it ready for review? |
@dukenv0307 you can review it |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-05.at.15.22.34.movAndroid: mWeb ChromeScreen.Recording.2024-10-05.at.15.20.38.moviOS: NativeScreen.Recording.2024-10-05.at.15.22.47.moviOS: mWeb SafariScreen.Recording.2024-10-05.at.15.19.44.movMacOS: Chrome / Safariweb-resize.mp4MacOS: DesktopScreen.Recording.2024-10-05.at.15.25.59.mov |
Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks! |
✋ 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/thienlnam in version: 9.0.46-0 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.46-5 🚀
|
Details
policy categories were not loaded on app start due to which the 'categorise it' button did not show policies that the user did not visit after login
Fixed Issues
$ #47854
PROPOSAL: #47854 (comment)
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
android_app.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.-.2024-09-25.at.22.27.17.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.-.2024-09-25.at.23.57.12.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-09-25.at.21.39.18.mp4
MacOS: Desktop
Screen.Recording.2024-09-25.at.21.44.05.mp4