fix: check NFC support and enabled state before NFC operations#3821
fix: check NFC support and enabled state before NFC operations#3821kaloudis merged 2 commits intoZeusLN:masterfrom
Conversation
|
/gemini summary |
|
/gemini review |
Summary of ChangesThis pull request significantly refines the NFC functionality across the application, addressing several critical issues related to NFC support and enabled states. It ensures a more robust and user-friendly experience by dynamically adapting the UI based on device capabilities and providing clear, actionable feedback when NFC is disabled, thereby preventing silent failures and crashes. Highlights
Changelog
Activity
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses several bugs related to NFC functionality by implementing checks for NFC support and its enabled state. The introduction of the checkNfcEnabled utility and the informative modal for when NFC is disabled on Android significantly improves the user experience. The fixes are consistently applied across various views, and the code is well-structured. I've included a couple of suggestions for minor improvements related to code duplication and UI consistency.
| <Button | ||
| title={localeString('general.cancel')} | ||
| onPress={() => toggleAndroidNfcModal(false)} | ||
| quaternary | ||
| ></Button> |
There was a problem hiding this comment.
For better UI consistency, consider making the 'Cancel' button quaternary when it's the only button available (i.e., when NFC is enabled). This was its style before this change. When NFC is disabled, making it a primary button next to the quaternary 'Open NFC Settings' button is a good choice to guide the user. Adding the quaternary prop conditionally would achieve this.
<Button
title={localeString('general.cancel')}
onPress={() => toggleAndroidNfcModal(false)}
quaternary={androidNfcModalIsNfcEnabled}
></Button>
There was a problem hiding this comment.
I think CANCEL should keep the style of other CANCEL buttons (when there are other options). Besides there is a margin problem if it is quaternary and the only button.
| const nfcSupported = await NfcManager.isSupported(); | ||
| this.setState({ nfcSupported }); |
There was a problem hiding this comment.
This logic to check for NFC support is also added in views/Send.tsx in this PR (and is also present in views/Receive.tsx). To avoid code duplication and improve maintainability, you could extract this into a reusable component, for example a Higher-Order Component (HOC) that would wrap these views and provide nfcSupported as a prop.
There was a problem hiding this comment.
The shared logic is two lines... A HOC would add substantially more boilerplate than it removes. A useNfcSupported hook would be the cleaner solution, but that requires migrating class components to functional components, which is out of scope here.
|
@myxmaster, Please rebase |
myxmaster
force-pushed
the
fix/nfc-availability-checks
branch
from
58aa838 to
691b842
Compare
ajaysehwal
left a comment
ajaysehwal
left a comment
There was a problem hiding this comment.
@myxmaster, I think we missed ChannelsPane.enableNfc for the NFC enabled check.
|
@myxmaster Another suggestion: the enableNfc method is duplicated across several components. I think we should extract it into a single shared implementation, perhaps in a separate PR. cc @kaloudis |
It is never called from anywhere, so I will remove it. And yes, in a separate PR I will centralize the |
5 participants
Description
Bugs fixes:
NfcManager.isSupported()was not checked before rendering the NFC icon/buttonModalStorewas declared inReceive's props interface but missing from the@inject(...)decorator, causing a crash when attempting NFC scanning in the advanced receive optionsChannelsPane.tsx(disableNfc,enableNfc)Changed behavior:
NfcManager.isSupported()Note:
On iOS
NfcManager.isEnabled()always returnstrue, so the disabled-NFC code path is Android-only. The iOS experience should be unchanged, please test this.This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: