io-app
io-app copied to clipboard
pagopa
feat(IT Wallet): [SIW-1090] Add NFC enable instructions screen
[!WARNING] This PR depends on https://github.com/pagopa/io-app/pull/5745
Short description
This PR adds the screen to display the instructions to enable the NFC for Android users.
List of changes proposed in this pull request
- Added
ItwAuthNfcInstructionsScreencomponent - Added required locale keys
How to test
On a simulator
Go to Profile > IT Wallet > Issuing, clicking on CIE + PIN you should be able to see the NFC instructions screen.
On a physical Android device
Disable the NFC, then go to Profile > IT Wallet > Issuing, clicking on CIE + PIN you should be able to see the NFC instructions screen.
Preview
Affected stories
- 🌟 SIW-1090: Come cittadino voglio sapere come attivare NFC per proseguire nel flusso di autenticazione
subtask of
- ⚡ SIW-435: Inizializzazione wallet
Generated by :no_entry_sign: dangerJS against d7463e18ff113ffc85082fa1804026077412f761
Codecov Report
Attention: Patch coverage is 9.09091% with 10 lines in your changes are missing coverage. Please review.
Project coverage is 48.86%. Comparing base (
4f204b4) to head (d7463e1). Report is 63 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #5733 +/- ##
==========================================
+ Coverage 48.42% 48.86% +0.43%
==========================================
Files 1488 1605 +117
Lines 31617 32242 +625
Branches 7669 7809 +140
==========================================
+ Hits 15311 15755 +444
- Misses 16238 16420 +182
+ Partials 68 67 -1
| Files | Coverage Δ | |
|---|---|---|
| ...features/itwallet/navigation/ItwStackNavigator.tsx | 66.66% <ø> (ø) |
|
| ts/features/itwallet/navigation/routes.ts | 100.00% <ø> (ø) |
|
| ...n/screens/ItwIdentificationModeSelectionScreen.tsx | 4.34% <0.00%> (ø) |
|
| ...screens/ItwIdentificationNfcInstructionsScreen.tsx | 14.28% <14.28%> (ø) |
... and 227 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 dataPowered by Codecov. Last update ba22fac...d7463e1. Read the comment docs.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
![dpulls[bot] avatar](https://avatars.githubusercontent.com/in/48063?v=4 "Published by a pub.dev verified publisher"){kind=small}
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See https://github.com/pagopa/io-app/pull/5733/commits/242f664238afe2573565cf0b91b78d10911c30d5 and https://github.com/pagopa/io-app/pull/5733/commits/654247801866d526120ab65c8a10aeac72b5a4b1
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR?
Beside this edge case, it seems to work fine now.
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR?
Beside this edge case, it seems to work fine now.
You are right. I didn't notice this edge case, I assumed that the user would activate the NFC after the navigation, so the useFocusEffect was enough. I think it's better to address this issue in this PR. Working on it!
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR?
Beside this edge case, it seems to work fine now.
After some testing I found that we can't tell if the user enables NFC from the notification bar settings..
According to this open issue, AppState onChange listener does not know if the notification bar has been pulled down, so the only way to refresh the NFC state is to wait for the user to go to another screen or app.
Some considerations:
- We could check the NFC status when the user presses the button and goes to the next screen. This will inevitably add some lag which will cause worse UX, imho.
- We could add NFC status polling, but I think this is an over-engineered solution for this case.
- In most cases the user discovers that their NFC is disabled after pressing the button. So the chances of a user turning NFC on/off from this screen are very low. That said, the probability exists, so we should keep this edge case in mind.
I think we can merge this PR and think about it later. What do you think?
Not related to the above issue: i pushed this commit to add the correct settings screen linking: https://github.com/pagopa/io-app/pull/5733/commits/80592f0d658883a58528a010978a965fec4fc291
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR? Beside this edge case, it seems to work fine now.
After some testing I found that we can't tell if the user enables NFC from the notification bar settings.. According to this open issue,
AppStateonChangelistener does not know if the notification bar has been pulled down, so the only way to refresh the NFC state is to wait for the user to go to another screen or app. Some considerations:
- We could check the NFC status when the user presses the button and goes to the next screen. This will inevitably add some lag which will cause worse UX, imho.
- We could add NFC status polling, but I think this is an over-engineered solution for this case.
- In most cases the user discovers that their NFC is disabled after pressing the button. So the chances of a user turning NFC on/off from this screen are very low. That said, the probability exists, so we should keep this edge case in mind.
I think we can merge this PR and think about it later. What do you think?
Not related to the above issue: i pushed this commit to add the correct settings screen linking: 80592f0
It's fine to leave this out from this PR.
Regarding the linking function, should we use openNFCSettings, as we already do in CieCardReaderScreen.tsx?
Thanks.
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR? Beside this edge case, it seems to work fine now.
After some testing I found that we can't tell if the user enables NFC from the notification bar settings.. According to this open issue,
AppStateonChangelistener does not know if the notification bar has been pulled down, so the only way to refresh the NFC state is to wait for the user to go to another screen or app. Some considerations:
- We could check the NFC status when the user presses the button and goes to the next screen. This will inevitably add some lag which will cause worse UX, imho.
- We could add NFC status polling, but I think this is an over-engineered solution for this case.
- In most cases the user discovers that their NFC is disabled after pressing the button. So the chances of a user turning NFC on/off from this screen are very low. That said, the probability exists, so we should keep this edge case in mind.
I think we can merge this PR and think about it later. What do you think? Not related to the above issue: i pushed this commit to add the correct settings screen linking: 80592f0
It's fine to leave this out from this PR. Regarding the linking function, should we use
openNFCSettings, as we already do inCieCardReaderScreen.tsx?Thanks.
Yes, could be better but does not respect what the screen says: Settings -> Search NFC -> Enable. @thisisjp should be ignore what the screen says and prefer a shorter path for the user?
The screen looks good but is it supposed to pop-up even when NFC is enabled? That's the behaviour I'm noticing on my Android device.
The screen was still using the wrong actions/selectors I was using for testing purposes. Now it should be working as expected. See 242f664 and 6542478
This approach doesn't seem to consider the edge case where you already have NFC enabled when landing on the authentication method screen and then you disable it. Pressing the "CIE + PIN" button will still yield a "Not implemented" alert. That's because we are dispatching the check when rendering the list. Do you think we should consider this edge case or can we leave this out in this PR? Beside this edge case, it seems to work fine now.
After some testing I found that we can't tell if the user enables NFC from the notification bar settings.. According to this open issue,
AppStateonChangelistener does not know if the notification bar has been pulled down, so the only way to refresh the NFC state is to wait for the user to go to another screen or app. Some considerations:
- We could check the NFC status when the user presses the button and goes to the next screen. This will inevitably add some lag which will cause worse UX, imho.
- We could add NFC status polling, but I think this is an over-engineered solution for this case.
- In most cases the user discovers that their NFC is disabled after pressing the button. So the chances of a user turning NFC on/off from this screen are very low. That said, the probability exists, so we should keep this edge case in mind.
I think we can merge this PR and think about it later. What do you think? Not related to the above issue: i pushed this commit to add the correct settings screen linking: 80592f0
It's fine to leave this out from this PR. Regarding the linking function, should we use
openNFCSettings, as we already do inCieCardReaderScreen.tsx? Thanks.Yes, could be better but does not respect what the screen says: Settings -> Search NFC -> Enable. @thisisjp should be ignore what the screen says and prefer a shorter path for the user?
My bad, that's true, I'd rather stick to the instructions on the screen. It makes sense. LGTM.
EDIT: We went for opening the NFC settings directly