Related to T2484 & T2506. Some users do not like the QR scanner popup, so an option has been added to the settings tab to turn off the scanner from appearing unless the button is clicked.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABC29f46e8cd7cf: [Cashtab] Hide QR popup setting
cd web/cashtab && npm start
using devtools, set the screen width to be smaller than 769px
navigate to the send tab
observe that the QR scanner popup appears
navigate to a send token screen
observe that the QR scanner popup appears
navigate to the settings tab
check 'hide qr scanner'
navigate to the send tab
observe that the QR scanner does not appear
navigate to a send token screen
observe that the scanner popup does not appear
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- auto-camera-open-setting
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 19360 Build 38457: Build Diff cashtab-tests Build 38456: arc lint + arc unit
Event Timeline
Comment Actions
The setting should also only be visible on mobile devices since it does not pertain to desktop devices
| web/cashtab/src/components/Common/Ticker.js | ||
|---|---|---|
| 32 ↗ | (On Diff #33939) | Instead of autoCameraOff: false autoCameraOn: true |
| web/cashtab/src/components/Configure/Configure.js | ||
| 1856 ↗ | (On Diff #33939) | Need this to be more clear. I think it's more intuitive to have the setting be: "Auto-open camera on Send" , to be cut to "Auto-open camera" at smaller screen width The setting should by default be on, since this is existing Cashtab behavior |
Comment Actions
Per review feedback, the cashtab setting has been switched from autoCameraOff to autoCameraOn, and the default value has been set to match existing cashtab behaviour. Wording has also changed to reflect review feedback.
Comment Actions
This diff looks good, arguably could land it as is. However, I think we need to get one more patch in first which will also help as we expand future settings options.
If you check out useWallet.js, ctrl+f "isValidCashtabSettings"
You'll see this logic:
// If you found an object in localforage at the settings key, make sure it's valid
if (isValidCashtabSettings(localSettings)) {
setCashtabSettings(localSettings);
return localSettings;
}
// if not valid, also set cashtabSettings to default
setCashtabSettings(currency.defaultSettings);
return currency.defaultSettings;So, when a user opens their wallet for the first time to this landed diff -- their settings will be invalid because they do not have this new parameter. Cashtab will bonk their settings back to default. Not such a big deal, but most Cashtab users are probably not using "USD" currency, and some future settings changes may make this a big deal.
What we need is a parseInvalidSettingsForMigration function that looks at the settings from local storage, determines if it is invalid because it is missing a parameter, adds the default for only that parameter, then retests for isValidCashtabSettings. If passes the retest, use that. Otherwise use default settings.
Please create a task to write this function and add it to this diff. It will probably be easier / practical for you to add that function in this diff. Since I've already reviewed here, it's okay to just add that function in this diff, since it solves the last problem holding this diff up.
Comment Actions
- See inline change.
- This function should be its own diff. We'll land it, then rebase this QR diff on top.
| web/cashtab/src/utils/validation.js | ||
|---|---|---|
| 153 ↗ | (On Diff #34081) | local forage modification should not be in the for loop; only do this one time after you have the correct object |
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.
Tail of the build log:
npm WARN EBADENGINE required: { node: '^12 || ^14 || >=16' },
npm WARN EBADENGINE current: { node: 'v15.14.0', npm: '7.7.6' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: 'postcss-preset-env@7.4.3',
npm WARN EBADENGINE required: { node: '^12 || ^14 || >=16' },
npm WARN EBADENGINE current: { node: 'v15.14.0', npm: '7.7.6' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE package: 'postcss-pseudo-class-any-link@7.1.1',
npm WARN EBADENGINE required: { node: '^12 || ^14 || >=16' },
npm WARN EBADENGINE current: { node: 'v15.14.0', npm: '7.7.6' }
npm WARN EBADENGINE }
npm WARN deprecated source-map-resolve@0.6.0: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated text-encoding@0.6.4: no longer maintained
npm WARN deprecated svgo@1.3.2: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm WARN deprecated axios@0.19.2: Critical security vulnerability fixed in v0.21.1. For more information, see https://github.com/axios/axios/pull/3410
npm WARN deprecated ts-custom-error@2.2.2: npm package tarball contains useless codeclimate-reporter binary, please update to version 3.1.1. See https://github.com/adriengibrat/ts-custom-error/issues/32
> cashtab@1.0.0 prepare
> cd ../.. && husky install web/cashtab/.husky
husky - Git hooks installed
added 1811 packages, and audited 1812 packages in 27s
199 packages are looking for funding
run `npm fund` for details
21 vulnerabilities (10 moderate, 10 high, 1 critical)
To address issues that do not require attention, run:
npm audit fix
To address all issues possible (including breaking changes), run:
npm audit fix --force
Some issues need review, and may require choosing
a different dependency.
Run `npm audit` for details.
npm notice
npm notice New major version of npm available! 7.7.6 -> 8.13.0
npm notice Changelog: <https://github.com/npm/cli/releases/tag/v8.13.0>
npm notice Run `npm install -g npm@8.13.0` to update!
npm notice
> cashtab@1.0.0 build
> node scripts/build.js
Creating an optimized production build...
Failed to compile.
src/utils/validation.js
Line 5:8: 'localforage' is defined but never used no-unused-vars
Search for the keywords to learn more about each error.
Build cashtab-tests failed with exit code 1