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

[IOCOM-905, IOCOM-906] New DS on Message Router #5755

Merged
merged 21 commits into from May 17, 2024
Merged

Conversation

Vangaorth
Copy link
Contributor

@Vangaorth Vangaorth commented May 9, 2024

Short description

This PR aligns the Message Router screen to the new DS.

iOS
Loading
iOS
Generic Error
iOS
Remote Content Error
Simulator Screenshot - iPhone 15 - 2024-05-17 at 15 05 43 Simulator Screenshot - iPhone 15 - 2024-05-17 at 15 09 47 Simulator Screenshot - iPhone 15 - 2024-05-17 at 15 05 37
Android
Loading
Android
Generic Error
Android
Remote Content Error
Screenshot_1715953471 Screenshot_1715953490 Screenshot_1715953580

List of changes proposed in this pull request

  • MessageRouterScreen aligned to the new DS

How to test

Using the io-dev-api-server, check the Message Router screen, in both its loading and failure states.

@pagopa-github-bot
Copy link
Collaborator

pagopa-github-bot commented May 9, 2024

Warnings
⚠️ Please include a Jira ticket at the beginning of the PR title

Example of PR titles that include Jira tickets:

  • single story: [PROJID-123] my PR title
  • multiple stories: [PROJID-1,PROJID-2,PROJID-3] my PR title

Generated by 🚫 dangerJS against 4958076

Copy link

codecov bot commented May 9, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 49.65%. Comparing base (4f204b4) to head (8fdb4ab).
Report is 86 commits behind head on master.

Current head 8fdb4ab differs from pull request most recent head 4958076

Please upload reports for the commit 4958076 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5755      +/-   ##
==========================================
+ Coverage   48.42%   49.65%   +1.22%     
==========================================
  Files        1488     1608     +120     
  Lines       31617    31939     +322     
  Branches     7669     7667       -2     
==========================================
+ Hits        15311    15858     +547     
+ Misses      16238    16027     -211     
+ Partials       68       54      -14     
Files Coverage Δ
ts/components/screens/LoadingScreenContent.tsx 88.88% <100.00%> (ø)
...features/messages/navigation/MessagesNavigator.tsx 15.38% <ø> (ø)
.../features/messages/screens/MessageRouterScreen.tsx 59.09% <60.00%> (+2.99%) ⬆️

... and 486 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 b4f2762...4958076. Read the comment docs.

# Conflicts:
#	ts/features/messages/screens/__tests__/__snapshots__/MessageRouterScreen.test.tsx.snap
Copy link
Contributor

@dmnplb dmnplb left a comment

Choose a reason for hiding this comment

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

What about hiding the header on error states? I know that @adelloste worked on this behavior through the react-navigation navigator. We'll probably discuss this at our next sync meeting so that we get consistent behavior between sections.

ts/components/screens/OperationResultScreenContent.tsx Outdated Show resolved Hide resolved
@Vangaorth
Copy link
Contributor Author

What about hiding the header on error states? I know that @adelloste worked on this behavior through the react-navigation navigator. We'll probably discuss this at our next sync meeting so that we get consistent behavior between sections.

Keep in mind that having the support functionality allows the user to report the problem. In case of a bad remote content configuration, if the user sends a report with a screenshot (of this screen's error), we can promptly know that and address the user towards the message sender instead of debugging our own systems. That's the main reason why we kept the header, in this first version.

@Vangaorth
Copy link
Contributor Author

What about hiding the header on error states? I know that @adelloste worked on this behavior through the react-navigation navigator. We'll probably discuss this at our next sync meeting so that we get consistent behavior between sections.

Also, what if the message loading takes forever? With no back button in the header, the user is stuck on a seemingly non-responsive application. By using it (the back button in the header), the user can cancel the operation and go back to the message list without having to wait for the system timeout (which may not occur at all in case of a very slow connection).

@Vangaorth
Copy link
Contributor Author

@shadowsheep1 for a check on the header support in LoadingScreenContent

@dmnplb
Copy link
Contributor

dmnplb commented May 17, 2024

By using it (the back button in the header), the user can cancel the operation and go back to the message list without having to wait for the system timeout (which may not occur at all in case of a very slow connection)

@Vangaorth That's why we have two different buttons. We can use the primary one to direct the user to the support flow (even with the precompiled fields, like message ID) and the second one to direct the user to the previous page ("Go back" or "Cancel"). We already manage a similar case in the app (payment flow). We can improve it in a second moment, btw.

@shadowsheep1 shadowsheep1 self-requested a review May 17, 2024 14:01
Copy link
Member

@shadowsheep1 shadowsheep1 left a comment

Choose a reason for hiding this comment

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

LGTM

@Vangaorth Vangaorth merged commit 8befc67 into master May 17, 2024
11 checks passed
@Vangaorth Vangaorth deleted the IOCOM-905-router branch May 17, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants