-
Notifications
You must be signed in to change notification settings - Fork 114
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
Migration of Profile Logic #1009
Migration of Profile Logic #1009
Conversation
migrating settings.auth out of general-settings and into ProfileSettings
logout now also uses a Modal in Profile Settings
moved endForceSync over to profile settings
removal of some old code in general-settings, take care of some formatting in ProfileSettings
also app version was migrated here, still some places that it listens to call in general-settings, so need to migrate those soon
now a modal pops up instead of an AlertBar to take user input and act accordingly -- hard to replicate and test behavior in the emulator so more testing needed
Discovered Platform.OS will always return web right now, so had to switch for accessing device type form cordova
leveraging the appConfig listener to update the settings once they are ready
mostly migrated consent checking now - put off the reconsent proceedure until I figure out state.go
now using a reactNative paper dates to handle downloading the data dump -- noting that we could switch to downloading a range when we update the ControlHelper
all settings are stored in state in ProfileSettings! So no longer need to import them from the scope of general-settings
this element shows the status of the permissions and now gets called when you check the app status in settings - eventually need to be able to launch conditionally like it is in general-settings - also need to add in the permissions bars
deleting old code in general-settings fixing spacings adding comments about migration fix issue with not using authSettings
i18n.language is now used to access the locale that get's used for the date picker
only allow enable button when overallStatus is good
A few issues that have come up during migration that I'm working on solving next:
Putting these off for later (discussion here)
Once these are handled, all the functionality in |
… profile_logic_migration
Logout was not logging the user out, it was just refreshing. Now the logout works
creating a React version of the permissions checks, once this is ironed out, it should be helpful in the initial login as well
Continuing to migrate away from angular, and ionic popups in particular, I've been trying to convert the Permissions screen that pops up into a React element. This is what it looks like so far. It is controlled in the @JGreenlee do you know of a way to get this setting up/loading to happen before the element is displayed? I'm not sure how React handles that when it comes to hidden elements, I'm showing/hiding it the same way I am with other popups in |
The duplicated modals are appearing one stacked on top of the other on the same screen, so there's no changing screens between having to deal with the two modals. I also don't see the console message after closing the first one, the modal's visibility is controlled by a boolean held in state. This is what that behavior looks like: DoubleModal.mov |
At what point in that video do you see two modals? I am confused. That seems fine-ish to me? Unless there are some clicks that are not obvious... |
At worst, it's a little annoying. There's two copies of the modal (one coming from ProfileSettings and one from LabelTab) and they both get tripped to open when there's a broken permission. They're one on top of the other, so I have to close them both, you can see in the video when the background color shifts is when the first modal closes. around 12s I close the first one, then at 14 I refresh & close the second modal -- so 2 "extra" clicks |
I am a bit tempted to merge with this given that it is currently broken very badly now and two clicks are better than no popup. And this is not a very frequent use case anyway |
We'll be able to fix the duplication as soon as we can implement React routing, but I'm not sure if there's a good way to share the state across components that are essentially independent of each other while we're in this middle zone. I've tried a custom hook, but that has separate instances for each place it is used I believe, and when I've tried to research most of what I've found would work if we were fully React, or we could potentially use someone's library (see first answer), but I haven't come up with anything that works in this context. The other option would be to merge the new modal on JUST the label screen, which I actually already tried earlier this morning on this branch. Let me know if that's preferred, and I'll test it more thoroughly and prepare a PR. Then we could maybe merge this PR right after the patches are on staging which I believe would maintain the timeline for React routing we were talking about earlier. |
@Abby-Wheelis So with this option, they would see the status modal popup on the label screen but would have to click on the button to launch it from the profile screen? That actually sounds like a good idea given that the label screen is (currently) the hardcoded default for when the app is launched. It is also consistent with the pre-react migration. pre-react, I don't think we popped up the status screen if people changed to profile - they had to click to launch |
On resume, only open the app status modal if I think that would be good enough. Variables on the |
@shankari yes. When something is broken, the modal will pop up regardless of screen, as long as they've visited the label screen before. And it works on a fresh launch because the label screen is the default. In Profile, clicking the button takes them to the old permissions page, but still works fine.
Ok, global is what I was looking for, let me try that out. |
prevent the modal from profile and the modal from label from opening at the same time suggestion from: e-mission#1009 (comment)
That worked! Thank you @JGreenlee Now, we have exactly 1 modal open
This resolved the modal-launching-if-broken oddness we've been chasing around, and so this should be good now and a fix to issue#967. The translate PR is also ready, and includes keys from the label screen translate#40 |
As Jack pointed out in review, I did not need this argument to useTranslation() that allowed me to condense the two parts into one as well
As Jack pointed out in review, we need more permissions on Android to copy text, so, if the platform is android we'll do nothing here to avoid breaking over insufficient permissions
ok, so I'm going to start reviewing this now... |
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 looks good overall and I have no qualms with merging and pushing to staging.
I would like to have the one non-future comment in here (remove action button that doesn't work) addressed before we move to production at the end of next week.
We can then come back and check the future comments and fix them after the switch to react routing.
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.
Going to merge this now because there is only one non-future change (removing the non-functional action button to change the notification time)
One more non-future change from: We need to make sure that the instruction text is correct for Android 13. e.g. for "unused apps disabled" are the instructions on how to resolve correct on android 13, or do we need to add a new
|
future comments have been noted in: Next Phase of migrating Profile to React |
Cleanup of now / near future comments: 🧹 Profile cleanup including the android text (3435e12) |
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.
For the most part, the changes represent moving away from angular and ionic code in order to implement things in React. We still use a lot of angular services, and the two log screens haven't been migrated yet.
AppStatusModal and PrivacyPolicyModal are two large pieces that are in Profile and Onboarding, so right now both versions are maintained, but it was my hope these would serve as a starting point when we get to the Onboarding migration.
import AlertBar from "./AlertBar"; | ||
import DataDatePicker from "./DataDatePicker"; | ||
import AppStatusModal from "./AppStatusModal"; | ||
import PrivacyPolicyModal from "./PrivacyPolicyModal"; |
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.
Addition of the AlertBar
-- which shows a "snackbar" at the bottom of the screen with a message in replacement of many ionic alerts -> cases include text copied, data pushed, etc.
DataDatePicker
used for choosing the data that json dump download is used for, replaces the ionic datepicker with react native paper's which is cleaner and works better
AppStatusModal
and PrivacyPolicyModal
we have talked about in the PR, so I'm sure you're up to speed on those, but were major changes here.
const { settings, logOut, viewPrivacyPolicy, | ||
fixAppStatus, forceSync, openDatePicker, | ||
eraseUserData, refreshScreen, endForceSync, checkConsent, | ||
dummyNotification, invalidateCache, showLog, showSensed, | ||
parseState, userDataSaved, userData, ui_config } = settingsScope; | ||
const { showLog, showSensed } = settingsScope; |
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.
significantly less dependent on the settingsScope
variables -- which means decreasing dependence on angular code, and approaching being able to use React routing
//conditional creation of setting sections | ||
let userDataSection; | ||
if(userDataSaved()) | ||
{ | ||
userDataSection = <ExpansionSection sectionTitle="control.user-data"> | ||
<SettingRow textKey="control.erase-data" iconName="delete-forever" action={eraseUserData}></SettingRow> | ||
<ControlDataTable controlData={userData}></ControlDataTable> | ||
</ExpansionSection>; |
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.
removed userData section in anticipation of eliminating calories from dashboard, and thus need to collect/maintain/show this data
<Appbar.Header statusBarHeight={12} elevated={true} style={{ height: 46, backgroundColor: 'white', elevation: 3 }}> | ||
<Appbar.Content title={t("control.profile")} /> | ||
<Text>{t('control.log-out')}</Text> | ||
<IconButton icon="logout" onPress={() => setLogoutVis(true)}></IconButton> | ||
</Appbar.Header> |
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.
addition of Appbar
is what allowed the logout button to go all the way to the top of the screen, and matches the label screen, characteristic of UI making progress into React
<AlertBar visible={dataPushedVis} setVisible={setDataPushedVis} messageKey='all data pushed!'></AlertBar> | ||
<AlertBar visible={invalidateSuccessVis} setVisible={setInvalidateSuccessVis} messageKey='success -> ' messageAddition={cacheResult}></AlertBar> | ||
<AlertBar visible={noConsentMessageVis} setVisible={setNoConsentMessageVis} messageKey='general-settings.no-consent-message'></AlertBar> | ||
|
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.
all of the Modal
and AlertBar
components replace ionic popups and their visibility is controlled by a state variable
This PR continues where #994 leaves off, working towards fully migrating the logic pieces of the Profile/Settings Page