Page MenuHomePhabricator

[Cashtab] Hide QR popup setting
ClosedPublic

Authored by kieran709 on Jun 9 2022, 21:01.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC29f46e8cd7cf: [Cashtab] Hide QR popup setting
Summary

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.

Test Plan

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

Repository
rABC Bitcoin ABC
Branch
auto-camera-open-setting
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 19360
Build 38457: Build Diffcashtab-tests
Build 38456: arc lint + arc unit

Event Timeline

bytesofman requested changes to this revision.Jun 9 2022, 22:00

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

This revision now requires changes to proceed.Jun 9 2022, 22:00

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.

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.

This revision now requires changes to proceed.Jun 13 2022, 22:01

responding to review feedback

  1. See inline change.
  2. 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

This revision now requires changes to proceed.Jun 22 2022, 18:22
kieran709 edited the test plan for this revision. (Show Details)

responding to review feedback

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

fixed no-unused-vars error

Rebased and updated validation tests to include new defaultSetting.

This revision is now accepted and ready to land.Jul 26 2022, 21:33
This revision was automatically updated to reflect the committed changes.