Settings (Change Name/Number/Password) #148
Conversation
…chen into AnnieW/firebase
…chen into AnnieW/firebase
…entralkitchen into kenneth/recaptcha
…ccentralkitchen into kenneth/recaptcha
There was a problem hiding this comment.
- Change phone number: I'm able to reset my phone number to a number that already exists as a different user. Definitely need to add something to check if the number is already in use.
- Name change not reflected in the drawer: I saw your msg in the channel, but are we going to do something about this, or should we try to address it in the language with the designs?
- All: the submit button doesn't become enabled until I hit "Done" on the keyboard, which was very confusing. It should probably enable immediately once the user starts typing (for name) or as soon as the content reaches the expected constraints/min character length).
I know this is going to continue to change with new designs so I didn't take a very close look at the code, but this is a great start. Thank you for taking initiative on this Kenneth!
…chen into kenneth/settings
…ave out of state). Cleanup/rename/etc
There was a problem hiding this comment.
I went through and renamed/refactored - partially to address @wangannie's comments.
The one comment I didn't address is for DrawerContent - I'm not sure why DrawerContent doesn't update; if @kennethlien does and can fix it that would be great!
Otherwise, changes to request:
- All function calls to functions from
request.jsare API calls to Airtable. That means you need to catch promise rejections when you callawait. I've done one for you; please be sure to log to Sentry as well. - Is there a reason to have the log out button on the settings page? (this is pending designs anyway, so nbd)
- Fix DrawerContent issue noted by Annie
Also, it kind of bothers me that settings/auth share all utils and styling, but I don't really think it can be helped other than possibly renaming everything (which is a pain).. I've refactored as much as I could.
For the issue with buttons not enabling, @kennethlien it's a case where the button enable/disable could be 'calculated' on-demand from the values in state. In these cases, keeping something like confirm in state that needs to be updated whenever X or Y changes in state is generally bad practice because it's easy to get inconsistencies, as we saw with what Annie brought up. I've refactored it to be similar to SignUpScreen, where the permission is calculated in render.
After that, I think functionality-wise it's good to go. Nice work!
There was a problem hiding this comment.
Great progress! The styling and everything is great 👍
Everything seems to work except for actually updating the display in the settings page immediately after changing something. I noted a potential temporary fix with reloading for that, but that kind of seems like it's ignoring a deeper issue, so I'd be interested in hearing what others think.
screens/settings/SettingsScreen.js
Outdated
| )} | ||
| <CategoryBar icon="info" title="About" /> | ||
| <Image | ||
| source={require('../../assets/images/blueprint.png')} |
There was a problem hiding this comment.
Ahh this looks pretty pixelated. @12aschen can we get a higher res version?
There was a problem hiding this comment.
Also we should include a link to learn more that goes to calblueprint.org
There was a problem hiding this comment.
Also I realized we should probably ask DCCK if they want to include a blurb in this About section
| } | ||
| }; | ||
|
|
||
| changeName = async () => { |
There was a problem hiding this comment.
The reload method I mentioned in Slack:
I added Updates.reload(); from import { Updates } from 'expo'; here.
If we modify the designs to warn users that it has to refresh for changes to take effect, this reload would probably need to happen in the confirmation stage instead. But also, if we have it there, the back button could get confusing, since it might imply that the change doesn't happen until you do the refresh, so if they go back, they might expect it to not change (and it visually won't change but it would in Airtable)... so maybe we could also move the updateCustomers to the confirmation too and make that the final step?? Sorry let me know if that makes no sense to you.
There was a problem hiding this comment.
Note: this requires using Updates from Expo SDK36 because the SDK37 version with expo-updates module gave me issues. It would perfectly fine in development, but when I tried it in production with expo publish, it gave me this error ENOENT: no such file or directory, open '/Users/anniewang/Development/dccentralkitchen/android/app/src/main/AndroidManifest.xml'. If you want to try the new expo-updates version, you can use this in the Expo dev tools window.
There was a problem hiding this comment.
my take: get rid of the back button on the success page, have button say 'click here to refresh' and some text above changed to 'refresh to see changes'
There was a problem hiding this comment.
Great work on this Kenneth!
Could you update the PR description with new screenshots and explain why we added Updates.reload(), and also create an issue about our concerns needing to eventually switch to using expo-updates for SDK37?
I ended up addressing a lot of the issues I commented on and went ahead and added a few things:
- Updated the About Blueprint language
- Updated the logo image to something higher res
- Added a link to location system settings
- Added a link to our privacy policy
- Added the app version number to display at the bottom
- Converted Log Out buttons to SettingsCard for consistency
- Added
Updates.reload()to all logout functions to address the issue with pages not refreshing on account changes (should hopefully take care of issues I was having in Fixed Logout and Readded Rewards Footer #149 ) - Some styling cleanup
There was a problem hiding this comment.
Styling updates look really good!
Some questions:
- Curious if we're going to add any additional icons or some sort of indicator for all the external links (privacy policy, "Click here to learn more at calblueprint.org", and the Report Issue link)
- Consistency with the capitalization in the subtitles(?) in the cards copy:
Change NamevsManage your location preferencesetc - Also is there any particular reason the "Log Out" pseudo-button looks like text? I feel like it looks very not-clickable even though it's red 😓
There was a problem hiding this comment.
Awesome stuff Kenneth! Left some comments for code quality and suggestions for navigation - all not paramount but nice to have. Functionality-wise, name change is working well, can't test phone number but believe that the other reviewers have it covered 👍🏼
Also went ahead and fix a single typo in AuthTextField that I noticed (unrelated to your PR).
This reverts commit 6844a56.
What's new in this PR
The widely requested "Settings" feature has now been implemented, and hopefully is mostly functional. From settings, a user can now change their name/number/password, the specifics are described below. The page leverages the design from resources.
AppNavigator.jsto accommodate a new button in the drawer titled "Settings".SettingsStack.jshas been created to add in respective screens that will be detailed.SettingsScreen.jshas sections dedicated to changing your name/number, updating/viewing privacy and location settings, an 'About' blurb for Blueprint, and logging out.NameChangeScreen.js, which usesAuthTextFieldto validate the name and then updates Airtable.PhoneNumberChangeScreen.js, which usesAuthTextFieldto validate the number, and relies onRecaptchaVerifierandVerificationModal.jsto text the number with a code before updating Airtable.TL:DR
The following have been added:
SettingsScreen.jsNameChangeScreen.jsPhoneNumberChangeScreen.jsSettingsStack.jsYou can now edit the core parts of your account.
How to review
Attempt to cause funny issues on Airtable, and break out of expected functionality. If testing adding a new phone number, please use a number you can text or the whitelist number: 6505555056, and the whitelist pin: 666666. If using the whitelist, please note you must complete the captcha regardless.
See the TL:DR for files to take a close look at.
Relevant Links
Related PRs
Dependent on Auth PR, since we're using some verification stuff to set to numbers.
Next steps/Known Issues
AppNavigator.jsis loaded early on, I had to work around that by only giving one option in settings while in guest mode.Screenshots
CC: @anniero98 @wangannie