Update RequestStepCategory to add Empty and Loading states for category list #41344

ZhenjaHorbach · 2024-04-30T15:56:03Z

Details

Not here page is briefly displayed when selecting a workspace

Fixed Issues

$ #40999
$ #41143
PROPOSAL: #40999 (comment)

Tests

Sing up with a new account
Create manual expense in Self DM
Create workspace
Navigate to the self DM
Click on 'Categorize it' under the new expense
Select the new workspace
Verify that the loader or list of elements are shown instead of Not here
Open workspace setting and open categories
Remove all items
Navigate to the self DM
Click on 'Categorize it'
Select the workspace with removed categories
Verify that the component is displayed indicating an empty list

Verify that no errors appear in the JS console

Offline tests

Sing up with a new account
Create manual expense in Self DM
Create workspace
Navigate to the self DM
Click on 'Categorize it' under the new expense
Select the new workspace
Verify that the loader or list of elements are shown instead of Not here
Open workspace setting and open categories
Remove all items
Navigate to the self DM
Click on 'Categorize it'
Select the workspace with removed categories
Verify that the component is displayed indicating an empty list

QA Steps

Sing up with a new account
Create manual expense in Self DM
Create workspace
Navigate to the self DM
Click on 'Categorize it' under the new expense
Select the new workspace
Verify that the loader or list of elements are shown instead of Not here
Open workspace setting and open categories
Remove all items
Navigate to the self DM
Click on 'Categorize it'
Select the workspace with removed categories
Verify that the component is displayed indicating an empty list

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

2024-05-08.21.25.42.mov

Android: mWeb Chrome

2024-05-08.21.25.42.mov

iOS: Native

2024-05-08.21.22.23.mov

iOS: mWeb Safari

ios.mov

MacOS: Chrome / Safari

2024-05-08.21.26.17.mov

MacOS: Desktop

2024-05-08.21.35.23.mov

melvin-bot · 2024-04-30T15:56:08Z

@mananjadhav 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]

ZhenjaHorbach · 2024-04-30T16:06:04Z

@mananjadhav

Hello )
PR is ready

@dannymcclain @shawnborton @dubielzyk-expensify

The only thing that needs to be clarified is designs
Can you help with this or confirm current changes?

Loader (Current version in my PR):

Empty list (Current version in my PR):

shawnborton · 2024-04-30T16:41:56Z

Should the empty list link to your workspace editor somehow? Otherwise it's not very helpful - it's just a dead end:

I would think we'd have a big green button at the bottom that might say "Edit categories" and it would navigate you to your workspace categories?

ZhenjaHorbach · 2024-04-30T20:50:14Z

Should the empty list link to your workspace editor somehow? Otherwise it's not very helpful - it's just a dead end:

I would think we'd have a big green button at the bottom that might say "Edit categories" and it would navigate you to your workspace categories?

Something like this ?

2024-04-30.22.49.40.mov

dubielzyk-expensify · 2024-04-30T23:56:48Z

I like that, though I'd suggest we should use the label Add categories or Edit categories. But maybe my English is failling here? Edit category seems like an odd label when you don't have any of them

ZhenjaHorbach · 2024-05-01T00:58:56Z

I like that, though I'd suggest we should use the label Add categories or Edit categories. But maybe my English is failling here? Edit category seems like an odd label when you don't have any of them

I agree with you )
When I rewatched the video I didn't like it either
d5994c9#diff-2a9066512656fa29b1565e362127de5cc11ba95b28f143a9db26f76dc3750785R2019
And I updated the text to plural

shawnborton · 2024-05-01T12:22:23Z

Nice, I like it! And agree about making it Edit categories instead.

dannymcclain · 2024-05-02T14:03:20Z

I'm into it as well!

ZhenjaHorbach · 2024-05-03T09:42:52Z

@mananjadhav
Hello )
Since we no longer have design problems
PR is ready )

mananjadhav · 2024-05-03T18:30:00Z

Thanks @ZhenjaHorbach. Will finish the review by tomorrow.

mananjadhav · 2024-05-04T16:31:03Z

src/pages/workspace/categories/CategoryForm.tsx

@@ -28,9 +28,12 @@ type CategoryFormProps = {

    /** Function to validate the edited values of the form */
    validateEdit?: (values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_CATEGORY_FORM>) => FormInputErrors<typeof ONYXKEYS.FORMS.WORKSPACE_CATEGORY_FORM>;
+
+    /** Should go back after submitting the form */
+    isShouldGoBackOnSubmit?: boolean;


Prefixing should is enough to understand it's a boolean.

Suggested change

isShouldGoBackOnSubmit?: boolean;

shouldGoBackOnSubmit?: boolean;

mananjadhav · 2024-05-04T16:31:46Z

src/pages/workspace/categories/CategoryForm.tsx

@@ -60,9 +63,13 @@ function CategoryForm({onSubmit, policyCategories, categoryName, validateEdit}:
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.WORKSPACE_CATEGORY_FORM>) => {
            onSubmit(values);
            Keyboard.dismiss();
+            if (isShouldGoBackOnSubmit) {


in which scenarios would we not go back?

Actually these changes are not necessary
I added other changes related to navigation
Because categories are tied to workspace screen
And after reloading the page with open categories we always navigate to workspace screen

App/src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts

Line 61 in a991b39

[SCREENS.WORKSPACE.CATEGORIES]: [SCREENS.WORKSPACE.CATEGORY_CREATE, SCREENS.WORKSPACE.CATEGORY_SETTINGS, SCREENS.WORKSPACE.CATEGORIES_SETTINGS, SCREENS.WORKSPACE.CATEGORY_EDIT],

src/pages/iou/request/step/IOURequestStepCategory.tsx

mananjadhav · 2024-05-06T06:29:03Z

src/pages/workspace/categories/CategorySettingsPage.tsx

@@ -39,6 +39,7 @@ function CategorySettingsPage({route, policyCategories}: CategorySettingsPagePro
    const {translate} = useLocalize();
    const {windowWidth} = useWindowDimensions();
    const [deleteCategoryConfirmModalVisible, setDeleteCategoryConfirmModalVisible] = useState(false);
+    const backTo = route.params?.backTo ?? '';


do we need a fallback string?

It doesn't really affect anything
So I think not)
I'll remove it now

mananjadhav

Just finished with the review, overall code changes look fine. I'll need a day to test this.

mananjadhav · 2024-05-07T07:41:46Z

@ZhenjaHorbach Can you please resolve conflicts? I'll get started on the checklist right after.

ZhenjaHorbach · 2024-05-07T07:54:15Z

@ZhenjaHorbach Can you please resolve conflicts? I'll get started on the checklist right after.

Hello )
I resolved conflicts

Beamanator · 2024-05-27T07:41:56Z

@ZhenjaHorbach thanks for pulling main, can 1 or both of y'all please confirm this is still looking good after pulling main? 🙏 We should be able to get this merged early this week if so! (since the freeze is melting)

…ymbols

ZhenjaHorbach · 2024-05-27T11:16:41Z

@ZhenjaHorbach thanks for pulling main, can 1 or both of y'all please confirm this is still looking good after pulling main? 🙏 We should be able to get this merged early this week if so! (since the freeze is melting)

Just checked
Found a specific bug that we didn't notice
And fixed it
Everything works fine now)

Beamanator · 2024-05-28T14:01:09Z

@ZhenjaHorbach do you mind letting us know what the bug was & making sure it's in the test steps? 🙏

@mananjadhav can you please re-review? We're ready to merge this once it's all approved, no more freeze for this PR!

ZhenjaHorbach · 2024-05-28T14:09:18Z

@ZhenjaHorbach do you mind letting us know what the bug was & making sure it's in the test steps? 🙏

@mananjadhav can you please re-review? We're ready to merge this once it's all approved, no more freeze for this PR!

If we create category with non-Latin characters(ЙЦУКЕ) or special symbols and then we'll try to edit the category name we get not found page
Because we need add parser for new routers and decode URI with non-Latin or special symbols

475d2bd#diff-d959b56c2ea536627740ef206f2a473588b85ea215fa5ebf1b5ae628e9ce722aR542

Like here

App/src/libs/Navigation/linkingConfig/config.ts

Lines 397 to 402 in 96425fa

    
           [SCREENS.WORKSPACE.CATEGORY_EDIT]: { 
        
               path: ROUTES.WORKSPACE_CATEGORY_EDIT.route, 
        
               parse: { 
        
                   categoryName: (categoryName: string) => decodeURIComponent(categoryName), 
        
               }, 
        
           },

mananjadhav · 2024-05-29T04:24:34Z

I'll do another round of review + testing today.

ZhenjaHorbach · 2024-05-30T13:45:54Z

@mananjadhav
Friendly bump )

mananjadhav · 2024-05-30T13:49:28Z

I could only test on Web yesterday. I need to test on mobile atleast once. Please give me a few hours, I'll post an update once done.

mananjadhav · 2024-06-02T06:13:46Z

Apologies for the delay. Tested in Web, 1 mWeb and 1 native. Attached videos for Web and Android.

android-categories.mov

web-categories-1.mov

web-categories-2.mov

Beamanator

LGTM! Merging cuz this has been pending for too long, but i think we should clean up the small comment i mentioned

Beamanator · 2024-06-03T07:17:47Z

src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

+        if (backTo) {
+            Navigation.navigate(ROUTES.SETTINGS_CATEGORY_SETTINGS.getRoute(policyId, category.keyForList, backTo));
+            return;
+        }
        Navigation.navigate(ROUTES.WORKSPACE_CATEGORY_SETTINGS.getRoute(policyId, category.keyForList));


NAB - maybe in a follow-up we can clean this up by passing backTo always? so we don't have so many "if backTo exists, pass it, otherwise don't" checks? I also think it looks like ROUTES.WORKSPACE_CATEGORY_SETTINGS.getRoute already handles backTo existing or not, right?

WORKSPACE_CATEGORY_SETTINGS does not handle backTo
Because it doesn't make sense
Since in the case of WORKSPACE_CATEGORY_SETTINGS the previous screen (WORKSPACE_CATEGORIES) will take up fullScreen

And it will be always previous (even when we copy link and open in new tab)

App/src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts

Line 61 in a991b39

[SCREENS.WORKSPACE.CATEGORIES]: [SCREENS.WORKSPACE.CATEGORY_CREATE, SCREENS.WORKSPACE.CATEGORY_SETTINGS, SCREENS.WORKSPACE.CATEGORIES_SETTINGS, SCREENS.WORKSPACE.CATEGORY_EDIT],

But the idea behind my changes was
What if we have backTo
This means that we opened CATEGORY_SETTINGS not from Workspace Screen
And from another place
What does it mean to use new routes and save history using backTo

Oh geez i missed that one is SETTINGS_CATEGORY_SETTINGS, one is WORKSPACE_CATEGORY_SETTINGS. Please forgive me 😅

OSBotify · 2024-06-03T07:27:32Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-06-06T12:33:34Z

🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.79-11 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

jjcoffee · 2024-06-26T09:34:15Z

src/pages/iou/request/step/IOURequestStepCategory.tsx

+                <View style={[styles.flex1]}>
+                    <BlockingView
+                        icon={Illustrations.EmptyStateExpenses}
+                        iconWidth={variables.modalTopIconWidth}


Coming from #43064. This caused a regression where the empty folder icon was not centred. This is because the width and height values used here are incorrect for the icon used.

filip-solecki · 2024-07-02T11:14:06Z

src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

                />
            </View>
        );
    };

-    const isLoading = !isOffline && policyCategories === undefined;
+    const isLoading = !isOffline && policyCategories === null;


@ZhenjaHorbach why did you change it? Right now loading state is not working properly. See here. Reverting this change fixes the issue

This is a known issue
But yes, I may have tested some functionality
But then I forgot to remove these changes 😅

Coming from #44224. This caused a regression that a loader is not displayed
To fix this we need to use undefined in case of an empty onyx state as in other places

Not here page is briefly displayed when selecting a workspace

12f4ec1

ZhenjaHorbach requested a review from a team as a code owner April 30, 2024 15:56

melvin-bot bot requested review from mananjadhav and removed request for a team April 30, 2024 15:56

ZhenjaHorbach added 2 commits April 30, 2024 22:55

Add edit button for IOURequestStepCategory

e063101

Update translations

d5994c9

Fix conflicts

2aa7ea0

mananjadhav reviewed May 4, 2024

View reviewed changes

src/pages/iou/request/step/IOURequestStepCategory.tsx Show resolved Hide resolved

ZhenjaHorbach added 3 commits May 5, 2024 12:38

Update param name shouldGoBackOnSubmit

c4ac3d8

Update navigation for categories

43b7f5e

Update branch and fix conflicts

a991b39

mananjadhav reviewed May 6, 2024

View reviewed changes

Remove fallback strings from backTo

9cd6a2f

Update branch and fix conflicts

8796829

Update branch and fix conflicts

146b21e

ZhenjaHorbach dismissed Beamanator’s stale review via e9d8c85 May 26, 2024 21:00

Fix bug with category_settings when using categoryName with special s…

475d2bd

…ymbols

Fix conflicts and update branch

c935985

ZhenjaHorbach added 2 commits May 30, 2024 23:20

Fix conflicts and update branch

f61e06c

Fix conflicts and update branch

66427c1

mananjadhav approved these changes Jun 2, 2024

View reviewed changes

melvin-bot bot requested a review from Beamanator June 2, 2024 06:14

Beamanator approved these changes Jun 3, 2024

View reviewed changes

Beamanator merged commit ae12459 into Expensify:main Jun 3, 2024
17 checks passed

github-actions bot mentioned this pull request Jun 4, 2024

Deploy Checklist: New Expensify 2024-06-04 #43037

Closed

m-natarajan mentioned this pull request Jun 4, 2024

[HOLD for payment 2024-06-28] [$250] Web - Categorizing - Offline indicator appears in Edit categories RHP on web layout #43062

Closed

6 tasks

lanitochka17 mentioned this pull request Jun 4, 2024

[HOLD for payment 2024-06-28] [$250] Web - Categorizing - Folder icon in empty state screen is not centered #43064

Closed

6 tasks

blimpich mentioned this pull request Jun 4, 2024

[$250] Track Expense - Hmm..not here page on selecting category with no existing workspace #43073

Closed

6 tasks

This was referenced Jun 6, 2024

[HOLD for payment 2024-06-13] [$500] Workspace - Not here page is briefly displayed when selecting a workspace #40999

Closed

[HOLD for payment 2024-06-13] [$250] Track - Categorize it - Not here page opens when selecting a workspace with no category #41143

Closed

jjcoffee reviewed Jun 26, 2024

View reviewed changes

filip-solecki reviewed Jul 2, 2024

View reviewed changes

ZhenjaHorbach mentioned this pull request Jul 22, 2024

[Pay meow][$250] "you haven't created any categories" appear for a moment when opening categories in workspace settings #44224

Closed

6 tasks

twilight2294 mentioned this pull request Oct 3, 2024

[HOLD for payment 2024-11-01] [$500] Tags - Background report changes to Tags page after clicking Custom tag name in RHP #49883

Closed

6 tasks

truph01 mentioned this pull request Oct 5, 2024

[HOLD for payment 2024-12-07] [$250] Pay Someone - Wallet page opens when refreshing in "Validate your account" page #50285

Closed

6 tasks

	isShouldGoBackOnSubmit?: boolean;
	shouldGoBackOnSubmit?: boolean;

Update RequestStepCategory to add Empty and Loading states for category list #41344

Update RequestStepCategory to add Empty and Loading states for category list #41344

Conversation

ZhenjaHorbach commented Apr 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Apr 30, 2024

ZhenjaHorbach commented Apr 30, 2024 • edited Loading

shawnborton commented Apr 30, 2024

ZhenjaHorbach commented Apr 30, 2024

dubielzyk-expensify commented Apr 30, 2024

ZhenjaHorbach commented May 1, 2024 • edited Loading

shawnborton commented May 1, 2024

dannymcclain commented May 2, 2024

ZhenjaHorbach commented May 3, 2024

mananjadhav commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach May 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach May 6, 2024 • edited Loading

Choose a reason for hiding this comment

mananjadhav left a comment

Choose a reason for hiding this comment

mananjadhav commented May 7, 2024

ZhenjaHorbach commented May 7, 2024 • edited Loading

Beamanator commented May 27, 2024

ZhenjaHorbach commented May 27, 2024

Beamanator commented May 28, 2024

ZhenjaHorbach commented May 28, 2024 • edited Loading

mananjadhav commented May 29, 2024

ZhenjaHorbach commented May 30, 2024

mananjadhav commented May 30, 2024

mananjadhav commented Jun 2, 2024

Beamanator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Jun 3, 2024

OSBotify commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZhenjaHorbach commented Apr 30, 2024 •

edited

Loading

ZhenjaHorbach commented Apr 30, 2024 •

edited

Loading

ZhenjaHorbach commented May 1, 2024 •

edited

Loading

ZhenjaHorbach May 5, 2024 •

edited

Loading

ZhenjaHorbach May 6, 2024 •

edited

Loading

ZhenjaHorbach commented May 7, 2024 •

edited

Loading

ZhenjaHorbach commented May 28, 2024 •

edited

Loading

ZhenjaHorbach Jun 3, 2024 •

edited

Loading

ZhenjaHorbach Jul 2, 2024 •

edited

Loading