feat: [IOPID-1724,IOPID-1726,IOPID-1727] Revamp CieCardReaderScreen
#5773
Conversation
pagopa-github-bot
changed the title
CieCardReaderScreenCieCardReaderScreen
Affected stories
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5773 +/- ##
==========================================
+ Coverage 48.42% 49.74% +1.31%
==========================================
Files 1488 1629 +141
Lines 31617 32383 +766
Branches 7669 7888 +219
==========================================
+ Hits 15311 16108 +797
+ Misses 16238 16212 -26
+ Partials 68 63 -5
... and 557 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Done! |
|
Hi @shadowsheep1
|
| }, [navigation]); | ||
|
|
||
| const navigateToAuthenticationScreen = React.useCallback(() => { | ||
| navigation.reset({ |
There was a problem hiding this comment.
In app there are few occurrences of navigation.reset (even in similar situations) do you think it is appropriate to use this feature and also ask other developers to make it a best practice?
There was a problem hiding this comment.
do you think it is appropriate to use this feature
Using reset depends on the specific flow/use case. Here I only extracted the navigation we actually have from the deprecated use of NavigationService in a JSX Component, where we should use navigation, coming from useNavigation(), instead.
ask other developers to make it a best practice
I don't. As I said before using reset instead of other kind of navigations depends on the specific use case/flow. Remember that we also have an architecture with multiple navigation stack, where the main stack changes according to some conditions, so I don't think that reset would be applicable in a more generic way, but in this specific case, reset works great.
I don't know if I've answered your doubts. What do you think?
Good catch 🚀! For the Android part it's a race condition with the modal presentation transition and the virtual keyboard opening. I guess we'll have this kind of behavior in other situations if we leave things like they are. I suggest to remove the modal transition for Android: <Stack.Group
screenOptions={{
gestureEnabled: false,
headerShown: false,
...Platform.select({
ios: TransitionPresets.ModalSlideFromBottomIOS,
default: undefined
})
}}
>Android.mp4What do you think? For the iOS part, this kind of message is hardcoded in the cie-ios-sdk, and we cannot configure it from RN side. A more structural refactoring of the EIC SDK, should be done. This is not subject of this PR and, as you know, a total refactor of this SDK is in waiting to be sure not to do the same work in multiple stream. |
Yes, the change for Android works for me! We can proceed! |
shadowsheep1
deleted the
IOPID-1724-1726-1727-revamp-cie-card-reader-screen
branch
Short description
This PR revamps the
CieCardReaderScreenwith the new DS.Warning
In this PR there's a descoping tracked with this JIRA task.
Demo
Flow
-.cie.login.mp4
-.cie.login.mp4
-.extended.adpu.mp4
-.cie.login.MP4.MP4
-.cie.login.MP4
a11y
-.cie.login.-.a11y.mp4
-.cie.login.-.a11y.MP4.MP4
List of changes proposed in this pull request
Removals
Additions
useInteractiveElementDefaultColorNamehook and pass theblueColorNametoCieCardReaderScreenthat, for now, remains aClasscomponent.Note
CieUnexpectedErrorScreenis rendered on those EIC SDK events:and
CieWrongCardScreenis shown when we have those events:while
CieExtendedApduNotSupportedScreenis visible for:"you have withdrawn you card, please retry and hold it still..." message, is now only visible during those events:
Modifications
Tip
The a11y focus on
CieCardReaderScreenis handled only for Android, like it is also the a11y announcements (governed by thecontentstate property, which is set only on Android), because iOS handles a11y focus through the system NFC bottom sheet.How to test
Run the app against physical devices, both Android and iOS, and test the EIC login flow. Try set a wrong EIC PIN, try to withdraw the EIC during the reading process, and so on...