fix: [PE-725] CGN merchant categories list details transition header #6313

LeleDallas · 2024-10-21T05:56:46Z

Short description

This pull request includes several changes to the CgnMerchantCategoriesListScreen.tsx and categories.ts files to improve the handling of the first render state and loading indicators.

List of changes proposed in this pull request

Update CGN categories store loading state to noneLoading
Ensure the RefreshControl component does not appear on the first render.

How to test

Enter into CGN detail screen
Press on Scopri le opportunità
Check the list transition

pagopa-github-bot · 2024-10-21T05:59:00Z

Affected stories

🐞 PE-725: Comportamento anomalo header in transizione pagina (Da Dettaglio carta CGN a Lista opportunità)
subtask of
- ⚡ PE-672: CGN - Bug fixing

Generated by 🚫 dangerJS against 25044df

codecov · 2024-10-21T06:13:22Z

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 46.96%. Comparing base (4f204b4) to head (25044df).
Report is 634 commits behind head on master.

Files with missing lines	Patch %	Lines
...eens/merchants/CgnMerchantCategoriesListScreen.tsx	0.00%	10 Missing ⚠️
...n/components/merchants/CgnMerchantListSkeleton.tsx	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6313      +/-   ##
==========================================
- Coverage   48.42%   46.96%   -1.47%     
==========================================
  Files        1488     1803     +315     
  Lines       31617    36561    +4944     
  Branches     7669     8705    +1036     
==========================================
+ Hits        15311    17170    +1859     
- Misses      16238    19334    +3096     
+ Partials       68       57      -11

Files with missing lines	Coverage Δ
...n/components/merchants/CgnMerchantListSkeleton.tsx	`0.00% <0.00%> (ø)`
...eens/merchants/CgnMerchantCategoriesListScreen.tsx	`2.22% <0.00%> (ø)`

... and 1426 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffdc0ea...25044df. Read the comment docs.

Hantex9

Essentially, the way it has been implemented solves the issue of the refresh indicator at the startup of the screen. However, by doing so, when a user performs a pull-to-refresh on that list, the refresh indicator doesn’t stay active while waiting for the response. It immediately disappears because the refreshing attribute always receives a false value as input.

To test this use case, you should run the dev-server and the IO app locally. Then, in the dev-server, modify the src/config.ts file and change the delay property, setting it to something like 2000 (which indicates a 2-second delay for all request to the dev-server). Afterward, go to that screen, and when performing a pull-to-refresh, you’ll notice that the refresh indicator doesn’t stay for 2 seconds but disappears immediately.

Since we want to show the refresh indicator only when the user performs a pull-to-refresh, I’ve outlined below some possible improvements you could apply and consider.

Let me know what do you think.

This is a preview with a delay of 3000ms on every HTTP request:

preview.mov

Hantex9 · 2024-10-21T13:13:09Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantCategoriesListScreen.tsx

      IOToast.error(I18n.t("global.genericError"));
    }
-    // eslint-disable-next-line functional/immutable-data
-    isFirstRender.current = false;
+    setIsFirstRender(false);


This line of code, if we track the isRefreshing state, isn't needed anymore

Hantex9 · 2024-10-21T13:13:43Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantCategoriesListScreen.tsx

@@ -130,7 +129,7 @@ export const CgnMerchantCategoriesListScreen = () => {
        renderItem={({ item, index }) => renderCategoryElement(item, index)}
        refreshControl={
          <RefreshControl
-            refreshing={pot.isLoading(potCategories)}
+            refreshing={isFirstRender && pot.isLoading(potCategories)}


Suggested change

refreshing={isFirstRender && pot.isLoading(potCategories)}

refreshing={isRefreshing && pot.isLoading(potCategories)}

Hantex9 · 2024-10-21T13:14:43Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantCategoriesListScreen.tsx


 export const CgnMerchantCategoriesListScreen = () => {
  const insets = useSafeAreaInsets();
-  const isFirstRender = React.useRef<boolean>(true);
+  const [isFirstRender, setIsFirstRender] = React.useState<boolean>(true);


Instead saving the state of the first render, since we want that the "refreshing indicator" will appears only when the user pulls down the list, what do you think about saving the state of isRefreshing and set it as default value to false?

Suggested change

const [isFirstRender, setIsFirstRender] = React.useState<boolean>(true);

const [isRefreshing, setIsRefreshing] = React.useState<boolean>(false);

ts/features/bonus/cgn/screens/merchants/CgnMerchantCategoriesListScreen.tsx

LeleDallas · 2024-10-22T10:30:07Z

@Hantex9 does this meet your expectations?
I’ve added a skeleton for the fetching mechanism (tested with 3 sec of delay)

Screen.Recording.2024-10-22.at.12.25.20.mov

Hantex9 · 2024-10-22T10:58:59Z

@Hantex9 does this meet your expectations? I’ve added a skeleton for the fetching mechanism (tested with 3 sec of delay)

Screen.Recording.2024-10-22.at.12.25.20.mov

Yes, this is exactly what I had in mind. The only thing I wanted to point out is that, in the video, it seems like the skeletons have too much horizontal padding and don't match the width of the displayed content. Other than that, everything is perfect in my opinion 🚀

Hantex9

Great work! Just a small improvement below, and everything else looks good.

Hantex9 · 2024-10-22T13:41:47Z

ts/features/bonus/cgn/screens/merchants/CgnMerchantCategoriesListScreen.tsx

+  const [isRefreshing, setIsRefreshing] = React.useState(false);
+  const [isPullRefresh, setIsPullRefresh] = React.useState(false);


Do we need two separate states to track this behavior? I believe we could manage with just one state that tracks the pull-to-refresh behavior. When the user opens this screen for the first time, the skeletons should always be shown as long as the pot is in a noneLoading state (i.e., both pot.isNone and pot.isLoading). About the the pull-to-refresh, having a separate state makes sense, but for the initial load, a single state should be enough if you check if the pot is none but in loading. What do you think?

Hantex9

LGTM! 🚀

…6313) ## Short description This pull request includes several changes to the `CgnMerchantCategoriesListScreen.tsx` and `categories.ts` files to improve the handling of the first render state and loading indicators. ## List of changes proposed in this pull request - Update CGN categories store loading state to `noneLoading` - Ensure the `RefreshControl` component does not appear on the first render. ## How to test - Enter into CGN detail screen - Press on _Scopri le opportunità_ - Check the list transition --------- Co-authored-by: Alessandro <[email protected]>

fix: merchant categories list loader condition

813a2a6

LeleDallas added the Carta Giovani Nazionale label Oct 21, 2024

LeleDallas requested a review from Hantex9 October 21, 2024 05:56

LeleDallas requested a review from a team as a code owner October 21, 2024 05:56

LeleDallas had a problem deploying to dev October 21, 2024 05:56 — with GitHub Actions Error

Merge branch 'master' into PE-725-CGN-details-transition-header

236bebd

LeleDallas temporarily deployed to dev October 21, 2024 05:57 — with GitHub Actions Inactive

Merge branch 'master' into PE-725-CGN-details-transition-header

c82b798

Hantex9 temporarily deployed to dev October 21, 2024 12:39 — with GitHub Actions Inactive

Hantex9 requested changes Oct 21, 2024

View reviewed changes

Merge branch 'master' into PE-725-CGN-details-transition-header

ebc7f38

Hantex9 temporarily deployed to dev October 21, 2024 13:25 — with GitHub Actions Inactive

Merge branch 'master' into PE-725-CGN-details-transition-header

e159214

Hantex9 temporarily deployed to dev October 22, 2024 10:59 — with GitHub Actions Inactive

LeleDallas added 2 commits October 22, 2024 14:57

refactor: hasIcon property in merchant skeleton

1e60f0a

refactor: add skeleton in merchant categories list

47e40a1

LeleDallas temporarily deployed to dev October 22, 2024 13:02 — with GitHub Actions Inactive

LeleDallas requested a review from Hantex9 October 22, 2024 13:02

Merge branch 'master' into PE-725-CGN-details-transition-header

8a80e56

LeleDallas had a problem deploying to dev October 22, 2024 13:28 — with GitHub Actions Error

LeleDallas temporarily deployed to dev October 22, 2024 13:28 — with GitHub Actions Inactive

Merge branch 'master' into PE-725-CGN-details-transition-header

ea10276

LeleDallas temporarily deployed to dev October 22, 2024 13:39 — with GitHub Actions Inactive

Hantex9 requested changes Oct 22, 2024

View reviewed changes

refactor: merchant categories refresh condition

b7a4bd7

LeleDallas had a problem deploying to dev October 22, 2024 14:15 — with GitHub Actions Error

LeleDallas temporarily deployed to dev October 22, 2024 14:15 — with GitHub Actions Inactive

Merge branch 'master' into PE-725-CGN-details-transition-header

25044df

LeleDallas temporarily deployed to dev October 22, 2024 14:18 — with GitHub Actions Inactive

LeleDallas requested a review from Hantex9 October 22, 2024 15:59

Hantex9 approved these changes Oct 23, 2024

View reviewed changes

LeleDallas merged commit 91fbb09 into master Oct 23, 2024
8 checks passed

LeleDallas deleted the PE-725-CGN-details-transition-header branch October 23, 2024 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [PE-725] CGN merchant categories list details transition header #6313

fix: [PE-725] CGN merchant categories list details transition header #6313

LeleDallas commented Oct 21, 2024

pagopa-github-bot commented Oct 21, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

Hantex9 left a comment •

edited

Loading

Hantex9 Oct 21, 2024

Hantex9 Oct 21, 2024

Hantex9 Oct 21, 2024

LeleDallas commented Oct 22, 2024

Hantex9 commented Oct 22, 2024

Hantex9 left a comment

Hantex9 Oct 22, 2024

Hantex9 left a comment

	refreshing={isFirstRender && pot.isLoading(potCategories)}
	refreshing={isRefreshing && pot.isLoading(potCategories)}

	const [isFirstRender, setIsFirstRender] = React.useState<boolean>(true);
	const [isRefreshing, setIsRefreshing] = React.useState<boolean>(false);

		const [isRefreshing, setIsRefreshing] = React.useState(false);
		const [isPullRefresh, setIsPullRefresh] = React.useState(false);

fix: [PE-725] CGN merchant categories list details transition header #6313

fix: [PE-725] CGN merchant categories list details transition header #6313

Conversation

LeleDallas commented Oct 21, 2024

Short description

List of changes proposed in this pull request

How to test

pagopa-github-bot commented Oct 21, 2024 • edited by jira bot Loading

Affected stories

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

Hantex9 left a comment • edited Loading

Choose a reason for hiding this comment

Hantex9 Oct 21, 2024

Choose a reason for hiding this comment

Hantex9 Oct 21, 2024

Choose a reason for hiding this comment

Hantex9 Oct 21, 2024

Choose a reason for hiding this comment

LeleDallas commented Oct 22, 2024

Hantex9 commented Oct 22, 2024

Hantex9 left a comment

Choose a reason for hiding this comment

Hantex9 Oct 22, 2024

Choose a reason for hiding this comment

Hantex9 left a comment

Choose a reason for hiding this comment

pagopa-github-bot commented Oct 21, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

Hantex9 left a comment •

edited

Loading