-
-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add scramble pin toggle #962
Conversation
@@ -340,24 +341,11 @@ export default class AddEditNode extends React.Component< | |||
enableTor | |||
}; | |||
|
|||
setSettings( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is setSettings
ripped out here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no purpose to this call as far as I can see, we're just getting the settings and setting it to the exact same settings. Which makes sense, when copying the node config nothing in the settings changes. The copied node is only added to settings once save node config is pressed on the copied configuration.
views/Settings/Security.tsx
Outdated
fontFamily: 'Lato-Regular' | ||
}} | ||
> | ||
{'Scramble PIN numbers'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be added to locales/en.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I'd recommend this be off by default and have it opt-in for a better UX |
@Bosch-0 I agree, but discussed with @kaloudis and we thought for now it should default to scrambled. Since all current users have scramble PIN on, we want to avoid defaulting to non-scrambled and unscrambling current users' pins. Ideally, we could design out a create pin flow that includes the scramble pin toggle so the user can just decide scrambled/non-scrambled off the bat. It also might make more sense to users if they could toggle scrambled on/off during pin creation and see the numbers on the pin pad get scrambled/unscrambled and decide which they prefer. |
Description
Adds toggle in Security settings to choose whether pin numbers should be scrambled.
Because all current users' pins are scrambled, made it so that default state is
scrambledPin = true
Also changed the Security Settings view to use a class instead of a function in order to remain consistent with the rest of the views, and also to make it easier to inject
SettingsStore
. Without injectingSettingsStore
I was running into bugs.This pull request is categorized as a:
Checklist
npm run tsc
and made sure my code compiles correctlynpm run lint
and made sure my code didn’t contain any problematic patternsnpm run prettier
and made sure my code is formatted correctlynpm run test
and made sure all of the tests passTesting
If you added new functionality or fixed a bug, 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
npm install
after this PR is merged inpackage.json
andpackage-lock.json
have been properly updatedOther: