👋 New Onboarding Flow + Switch to React Navigation #1032
Conversation
…ission-phone into react_navigation_new_onboarding
Instead of using Ionic's tabbing implementation, we're going to use `BottomNavigation` from RN Paper, included in a central App component. This App component will have an AppContext, which can include any app-wide variables/functions the tabs need to share, including the loaded dynamic config - this way we can just load it once and use it everywhere. So App.tsx is replacing main.html, as well as the logic that was in MainCtrl. The metrics tab is only shown in 'MULTILABEL' configurations (same as the implementation was in MainCtrl). Onboarding hasn't been implemented yet; so there is a placeholder for it right now
By default, the active tab at the bottom of the screen shows in secondaryContainer color, which for us is a light yellow/orange. For this app, it makes more sense for this to be light blue. primaryContainer is close, but it can be lightened a bit more to match the lightness of secondaryContainer. Because we use primaryContainer for TimestampBadges, these now sort of blend into the background because they're lighter. A better idea for this is to give an small outline that is 'primary' color. If we replace the Badge component with a View we can add this outline, and we also don't have to worry about the warnings that we got before with the Badge component. Also added some comments describing the style, color/height overrides for BottomNavigation
|
This is what the new tabs look like: 
|
to increase reusability of the privacy policy element, we need flexibility of how the component is formatted, achieve this by having the title and scrollview (with all the text) as one component, and the container as another. Chose to keep buttons with the container -- this allows for custom options between profile and onboarding -- policy is view only and can be "closed" from profile, but must be explicitly accepted or rejected in onboarding
in an attempt to keep the blame, I'm renaming and moving this to then cut it down to just the controls, and will re-establish the modal to hold the controls elsewhere
to increase flexibility, we need to separate the permissions modal from it's contents. SettingsScope is also no longer a needed argument
we need to show the appStatusModal whenever the overallStatus is false now that the controls are split from the modal, we need to have central access to the overallStatus usePermissionStatus controls all the permissions from a central location. overallStatus can be easily extracted to show the modal in AppStatusModal in PermissionsControls, we now extract the checkList, overallStatus, etc from usePermissionStatus in order to control the UI
in updateCheck, we should also update the statusIcon and statusColor as we replace the check in the list, this way the indicators stay in sync with the status this eliminates a lag I was observing between the checks updating and the icons showing, as well as prevents infinite looping observed when replacing the checks to update UI when checkList changes
no longer need a scope to listen for updates, as we are using usePermissionStatus to show the modal, and updating based on useAppStateChange
|
I have discovered that I think the most efficient thing to do here is just rewrite |
… into react_navigation_new_onboarding Additional changes needed to resolve merge conflicts and restore functionality.
This file is replacing the angular service DynamicConfig - several of the functions have been rewritten, preserving the same function while modernizing the code. The only notable differences with the new version are that there are no longer any `saveAndNotifyConfigChanged` or `saveAndNotifyConfigReady` functions, and routing is not handled here. These were things that relied heavily on Angular. This is no longer an Angular module - and this file should strive to be fairly agnostic of the framework. So the routing, and the handling of what to do when the config has updated, will be handled in the components that _use_ dynamicConfig's functions by checking the result of the returned promises.
This sketch of the new Join page is fairly rudimentary, but it includes the same text as the old one, and a text popup to enter an OPcode. It uses `initByUser` from the *new* dynamicConfig.ts file, not the old dynamic_config.js file. Qr scanning has not been implemented yet
We will derive onboarding state from the result of StartPrefs.getPendingOnboardingState - if it is null (no pending state) then we can route to the rest of the application. If there is a string value, it specifies which screen should be shown in the OnboardingStack. Currently, only JoinPage is implemented in OnboardingStack
Because Unix systems are case-insensitive to filenames, and Typescript is insensitive to file extensions when dealing with imports, we had a conflict between app.js and App.tsx. This was causing autoreload to not work for the App.tsx component. I'm going to rename the old app.js file, which is the Angular entry point, to ngApp.js. Soon, we will not need it anyway!
This will replace getPendingOnboardingState from the StartPrefs service. The new implementation will be typed according to the OnboardingState type and congruent with what OnboardingStack expects.
We don't have any Angular UI elements to be displayed anymore, but we still need Angular for the services. So we'll keep the initialization logic that is happening in ngApp.js, but remove <ion-nav-bar> and <ion-nav-view> from index.html. Instead we will have an #appRoot element, and at the end of the Angular init logic in ngApp, we'll inject the App.tsx component into this #appRoot. At this point, we shouldn't need to angularize / embed React views into Angular views, because there _are_ no Angular views anymore!
This is called on logout and if the user declines consent. It cleared the stored config, but it didn't clear consent. It should clear everything, so let's implement a new version on the rewritten version of dynamicConfig, which uses clearAll instead of clearing just the config.
This is a temporary ConsentPage with just 2 buttons: accept/decline. On accept we execute the logic that was in intro.js - it calls `login` and attempts to register the user via CommHelper. On decline, we clear data and force refresh
rewrites from server_conn - the function in serverConn for setServerConnSettings does the same as `init` from server_conn did, and will be called from App.tsx any time the dynamic config changes
Because the entire App component is now injected into a SafeAreaView in ngApp.js, we no longer need these offsets
Instead of including the AppStatusModal individually on the tabs, we can include in in the entire App component and have it appear any time permissions are incorrect. However, it should only show if the user already consented
was mistakenly exporting this component as PrivacyPolicyModal (the wrapped component used in ProfileSettings)
add PrivacyPolicy for users to accept needed flex:1 on the page style to enable scrolling
previously, the summary (in a list of three lines) was presented after entering an opcode and before showing the longer privacy policy for consent To simplify slightly, we add the summary to PrivacyPolicy Also moved scroll from PrivacyPolicy to it's containers to keep the consent button at the bottom of the scroll -- users have to at least scroll past what they're agreeing to
StateProvider is not needed anymore because we handle routing in React, between the App component and the OnboardingStack component JoinCtrl is not needed because the new onboarding flow has re-implemented all this functionality
The EnketoSurveyLaunch and EnketoSurvey services have been replaced. The EnketoModal component (EnketoModal.tsx) and the enketoHelper.ts file handle all the survey responsibilities that EnketoSurveyLaunch and EnketoSurvey previously handled
enketo-preview.js is not used anymore, nor is survey/external
|
While removing all the old 'intro' code, I pruned a ton of other unused code too, in hopes that it will make it easier to clean up and rewrite all the Angular services left scattered around. However, I'm going to stop here because this PR is already big and it will get harder to review the more work I do. |
|
I'm working on wrapping up QR scanning now, but haven't been able to get to the onboarding flow to test. When I log out I just end up stuck on a loading label screen with a permissions modal with no permissions. I've tried setting a breakpoint in the onboarding stack if/else ladder, but I never hit the breakpoint. |
We were running into an issue where the AppStatusModal showed in situations where it shouldn't. This is because if there are no checks loaded yet, overallStatus is false- so if AppStatusModal is initiated before the checks are ready, it will see that overallStatus is false and trigger the pop up. We will add extra criteria: for the popup to show, overallStatus must be false AND there must be at least one check in checkList
|
I am pretty sure this is due to an error in the new Update: fixed in 593248e |
I realized I used the wrong function from the BEMUserCache plugin; there is no such thing as getRWDocument; the function is just called getDocument. Also, when we get a config doc from native storage, we should not just make sure it's truthy but we should also ensure that it is not empty. So it must have >0 keys. Lastly, when a config is not found from KVStore nor user cache, this does fall in the realm of unexpected behavior (like upon the user logging out). We don't need to popup an error to the user. instead let's just log a warning to the console
restoring error messages to scanCode, using the "displayError" method from logger, as used by other parts of login in "initUser" Tested in the devapp, code scans and logs in properly, and if I scan a random qr code it throws an error popup
Despite being an image, this file used to be inside the /templates folder, which was deleted in e-mission@c5fbad4. The image is used by EnketoModal, so we need to add it back - this time, it will be located in /img.
in trialing in the devapp, I was getting errors when I tried the popOpCode modal - I resolved by adding the same style prop present in the SaveQR page where
|
^I can confirm that the fix worked for me, I was able to ensure that QR scanning is working, as well as restore error handling. Also @JGreenlee's new styling of the Welcome page and the whole onboarding flow looks awesome! |
|
Great! I can confirm that QR scanning is working in my testing too. Marked as ready! |
|
I can't believe we got this done in less than 1 week! Things are really coming together as we approach the end of the UI rewrite. I believe #1034 can be closed because it was absorbed into this PR. |
Yes, you are definitely on a roll here! It took a lot of initial work, but I think that we can see the light at the end of the tunnel (and I don't think it is a train 😄 ) |
Since 5e7a50c, we have React versions of ControlCollectionHelper and ControlSyncHelper that we include right here in e-mission-phone. So we no longer need any extra scripts to download these from other repos, and we do not need to include them in index.js anymore.
We had combined the study summary and privacy policy into one component. We're splitting them back up. They will both be located in js/onboarding.
StudySummary is now its own component separate from PrivacyPolicy. It uses the text that was previously in the first section of PrivacyPolicy. And PrivacyPolicy is now just what used to be the second section. These components share the function getTemplateText - the function is located in StudySummary, since the summary comes first, and imported in PrivacyPolicy to be used there too. getTemplateText was also defined on ConsentPage, but it doesn't need to be there as it's not handled there.
We split off the summary to be separate from the privacy policy, so the summary will now show on a separate page preceding the privacy/consent page. We will use a boolean flag in onboardingHelper, summaryDone, which is false initially, so after welcome is done, `getPendingOnboardingState` returns 'summary'. Once the "Proceed" button is clicked on the SummaryPage, summaryDone is true and `getPendingOnboardingState` will return 'consent'.
- have "Proceed" button show at the bottom by having the page take the full height and marginTop: auto on the button row -adjust spacing between text by grouping summary lines together and adding gap of 16 pixels on the SummaryPage Surface -make "Proceed" button 'contained', not 'outlined'
…ission/e-mission-phone into react_navigation_new_onboarding While merging this, I resolved some conflicts. On the current branch, usePermissionStatus was split off from AppStatusModal. But changes were made to AppStatusModal on the incoming branch. I had to carefully re-apply those changes to usePermissionStatus and AppStatusModal. The important consideration here is that we allow overallStatus to be undefined when any permissions status has not yet been determined. So in AppStatusModal, we explicitly check that `overallStatus === false` before showing the modal, rather than just checking that `!overallStatus`.
Since e25a4ff, 'osver' and 'platform' are not used anymore. We switched to accessing these directly from `window['device']` as `window['device'].version` and `window['device'].platform`.
|
Because of the upstream fixes from earlier today (#1041) we had some tricky merge conflicts here. |
There was a problem hiding this comment.
Given that this change involved 146 files, my code review is going to focus on high level structure and functionality and not on sanity checking code. Although, kudos to @JGreenlee and @Abby-Wheelis - the code is very clean, and I don't have any high priority fixes.
I am merging this right now to get a release out tonight for the accessibility testing. You can address the comments in a cleanup fix.
| body.platform-ios { | ||
| padding-top: calc(env(safe-area-inset-top) / 2); | ||
| margin: 0 10px 0 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
I added this to accommodate the "iOS top cutout", because otherwise it overlapped with the date selection (check the blame for details). Is that not an issue any more?
There was a problem hiding this comment.
Not needed because we are using SafeAreaView
| "welcome-to-app": "Welcome to <b>{{appName}}!</b>", | ||
| "app-name": "NREL OpenPATH", | ||
| "to-proceed-further": "To proceed further, please scan or paste an access code that has been provided by your program administrator through a website, email, text message, or printout.", | ||
| "code-hint": "The code begins with ‘nrelop’ and may be in barcode or text format.", |
There was a problem hiding this comment.
"QR code" instead of "barcode"?
barcode is generally a bar...
There was a problem hiding this comment.
A QR code is a type of barcode. But not everyone knows what a QR Code is (source: my parents).
There was a problem hiding this comment.
@JGreenlee I believe these png files are used by the native code as the icon for notifications.
For example
$ grep -r ic_mood_question plugins/
plugins//cordova-plugin-em-serversync/res/android/authenticator.xml: android:smallIcon="@drawable/ic_mood_question"
plugins//cordova-plugin-em-serversync/plugin.xml: <resource-file src="res/android/ic_mood_question.png" target="res/drawable/ic_mood_question.png"/>
Although, to be fair, that plugin also has this file
$ ls plugins/cordova-plugin-em-serversync/res/android/
authenticator.xml ic_mood_question.png syncadapter.xml
If we can confirm that the other files are also included from plugins, I am fine with removing the files. Otherwise, they must be retained.
There was a problem hiding this comment.
to answer my own question
$ grep -r ic_question_answer plugins/
$
so maybe this is OK after all.
| useEffect(() => { | ||
| if (!appConfig) return; | ||
| setServerConnSettings(appConfig).then(() => { | ||
| refreshOnboardingState(); | ||
| }); | ||
| }, [appConfig]); |
There was a problem hiding this comment.
Is this essentially code from www/js/config/server_conn.js?
If so, I am not sure that this should be in App.js.
Are we going to put in all the configuration options into App.js?
We are clearly not doing so for imperialConfig, for example.
There was a problem hiding this comment.
It has to be called from somewhere. The new serverConn.ts does not self-initialize - it is just a collection of functions.
Whether it is in the App component or not, we have to listen for config changes and call setServerConnSettings. Would you suggest a separate file for initialization logic? Besides this and the i18n initialization, I am not sure what would go in there.
The reason we do not have initialization logic here for imperialConfig is that it was made into a React hook and is used at the component-level.
useImperialConfig is used in a lot of different components. serverConn is just set up initially and not used by components.
| angular.module('emission.survey.enketo.answer', ['ionic']) | ||
| .factory('EnketoSurveyAnswer', function($http) { |
There was a problem hiding this comment.
I thought we had removed all dependencies on ionic
From #1032 (comment)
When this PR is finished, we should no longer have any reliance on Ionic.
There was a problem hiding this comment.
I don't think this file actually uses ionic and it could be removed as a dependency here.
But unfortunately, we are not fully done with ionic. After this PR we don't use any of the ion-* HTML elements. But many of the services that haven't been rewritten do still use $ionicPopup and $ionicPlatform.
There was a problem hiding this comment.
just for my edification, we have removed the reconsent logic completely, correct?
I remember discussing this with @Abby-Wheelis during the profile screen rewrite, but just wanted to check since I don't see a replacement.
There was a problem hiding this comment.
The last update I recall from the consent logic was that we should remove reconsent at some point, but we first needed to brainstorm a good replacement for the event that we updated some sort of terms mid-data collection. I have that task tracked in my issue for the "future profile updates" but we still need to brainstorm and decide on a replacement.
| return (<> | ||
| <ScrollView contentContainerStyle={{flex: 1}}> | ||
| <Surface style={[onboardingStyles.page, {flex:1, gap: 16}]}> | ||
| <StudySummary /> |
There was a problem hiding this comment.
why do we need to split StudySummary and SummaryPage? I thought we were not showing the StudySummary in the profile page, only the privacy policy.
There was a problem hiding this comment.
I thought we were not showing the
StudySummaryin the profile page, only the privacy policy.
That's correct as of now. But is there any chance this would change in the future?
| let text = result.text.split("=")[1]; | ||
| console.log("found code", text); | ||
| loginWithToken(text); |
There was a problem hiding this comment.
weren't we checking here to see that the QR code actually did start with emission://?
There was a problem hiding this comment.
That was somewhere in the older version of the QR scanning flow, you're right. I think I got focused on splitting the token from the rest of the url, I can add that back in the cleanup pass. Before, there was a line with result.text.startsWith("emission://"))
This PR unifies the 3 tabs we have migrated (Label, Dashboard, Profile) under a central component in React (the
Appcomponent).It utilizes
BottomNavigationfrom React Native Paper, described here:This PR is also where we will rewrite the onboarding flow, since it will use React navigation. New flow discussed here:
When this PR is finished, we should no longer have any reliance on Ionic.