Skip to content
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: [IOBP-838] Remove receipt from list #6334

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

LeleDallas
Copy link
Contributor

@LeleDallas LeleDallas commented Oct 25, 2024

This PR depends on

⚠️ This PR depends on #430 -> Please review the dependent PR before reviewing this ⚠️

Short description

This pull request introduces a new feature to hide receipts from the list

List of changes proposed in this pull request

  • Create a loading screen component for the payment section
  • Add hide button at the bottom of PaymentsTransactionBizEventsDetailsScreen
  • Update payment navigation route and saga
  • Apply optimistic UI logic to hide elements

How to test

Dev-server

  • In config.ts set a delay and edit the hideReceiptResponseCode property

  • Go to payments section
  • Tap on an element
  • Scroll to the bottom and tap on Nascondi dalla lista
  • Verify if the element disappears after the redirect.

Preview

Screen.Recording.2024-10-29.at.11.37.44.mov
Screen.Recording.2024-10-29.at.11.39.53.mov
Screen.Recording.2024-10-29.at.11.40.58.mov

@thisisjp would it make sense to set a minimum delay before displaying the Loading component? This could help avoid the component flashing on screen for very short loading times, leading to a smoother user experience.

@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented Oct 25, 2024

Affected stories

  • 🌟 IOBP-838: Come CIT voglio rimuovere una ricevuta dalla mia lista
    subtask of
    • IOBP-246: Evolutive sezione Pagamenti

Generated by 🚫 dangerJS against bda2400

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 22.22222% with 49 lines in your changes missing coverage. Please review.

Project coverage is 46.96%. Comparing base (4f204b4) to head (bda2400).
Report is 670 commits behind head on master.

Files with missing lines Patch % Lines
...ments/bizEventsTransaction/store/reducers/index.ts 0.00% 21 Missing ⚠️
...n/saga/handleDisableBizEventsTransactionReceipt.ts 0.00% 15 Missing ⚠️
.../components/PaymentsBizEventsHideReceiptButton.tsx 7.69% 12 Missing ⚠️
...atures/payments/bizEventsTransaction/saga/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6334      +/-   ##
==========================================
- Coverage   48.42%   46.96%   -1.46%     
==========================================
  Files        1488     1819     +331     
  Lines       31617    37001    +5384     
  Branches     7669     8807    +1138     
==========================================
+ Hits        15311    17379    +2068     
- Misses      16238    19565    +3327     
+ Partials       68       57      -11     
Files with missing lines Coverage Δ
...ents/bizEventsTransaction/navigation/navigator.tsx 66.66% <ø> (ø)
...payments/bizEventsTransaction/navigation/routes.ts 100.00% <ø> (ø)
...eens/PaymentsTransactionBizEventsDetailsScreen.tsx 8.10% <ø> (ø)
...yments/bizEventsTransaction/store/actions/index.ts 100.00% <100.00%> (ø)
...tures/payments/bizEventsTransaction/utils/index.ts 76.92% <100.00%> (ø)
...atures/payments/bizEventsTransaction/saga/index.ts 0.00% <0.00%> (ø)
.../components/PaymentsBizEventsHideReceiptButton.tsx 7.69% <7.69%> (ø)
...n/saga/handleDisableBizEventsTransactionReceipt.ts 0.00% <0.00%> (ø)
...ments/bizEventsTransaction/store/reducers/index.ts 8.82% <0.00%> (ø)

... and 1438 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 f620db4...bda2400. Read the comment docs.

Copy link
Contributor

@Hantex9 Hantex9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, It looks good to me! Maybe consider adding a js-doc to the utility function. After that, you're free to merge the PR 👍

index: number
): NoticeListItem => pot.getOrElse(transactions, [])[index];

export const restoreTransactionAtIndex = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be useful for a jsdoc to explain the purpose of this utility function better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants