-
Notifications
You must be signed in to change notification settings - Fork 105
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
refactor: [IOBP-1091] Paid in app empty state screen #6575
Conversation
Jira Pull Request LinkThis Pull Request refers to the following Jira issue IOBP-1091 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6575 +/- ##
==========================================
- Coverage 49.29% 49.29% -0.01%
==========================================
Files 1555 1555
Lines 32119 32122 +3
Branches 7312 7268 -44
==========================================
Hits 15834 15834
- Misses 16236 16250 +14
+ Partials 49 38 -11
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
const emptyProps: { | ||
title: string; | ||
subtitle?: string; | ||
pictogram: IOPictograms; | ||
} = |
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.
I noticed you’ve defined a custom type for emptyProps. Since these properties are closely tied to OperationResultScreenContent
, I suggest creating a derived type using Pick
. This will make the code more maintainable and consistent with the component’s actual props. Example:
type OperationResultEmptyProps = Pick<
typeof OperationResultScreenContent.propTypes,
"title" | "subtitle" | "pictogram"
>;
This also helps avoid duplicating definitions if the props change in the future. Let me know what do you think about.
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.
What do you think to use OperationResultScreenContentProps
type instead of typeof OperationResultScreenContent.propTypes
?
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.
Yes, it makes more sense. I saw that the type OperationResultScreenContentProps
was not exported. So in this case it's better to export it and use it 👍
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.
You can find it here d9cbd7f
type OperationResultEmptyProps = Pick< | ||
OperationResultScreenContentProps, | ||
"title" | "subtitle" | "pictogram" | ||
>; |
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.
Could you declare the type definition outside the component hook?
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.
Sure!
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.
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!
Short description
This pull request includes updates to localization files and enhancements to the
ReceiptListScreen
component. The most important changes is refactoring the empty state logic in the receipt list screen.List of changes proposed in this pull request
emptyProps
object based on thenoticeCategory
.emptyPayer
locale for displaying payment receipts made in the app.How to test
Pagamenti
tap onVedi tutte
Pagate in app
tab is display correct empty statePreview
Screen.Recording.2024-12-20.at.10.06.52.mov