More profile migrations #994
Conversation
This is not yet complete, but is an idea of where the schedule table might eventually be
incorporating a patch from Jack with a solution to some of the issues with scheduled notifications, as well as cleaning up some of the style around the control-data-table (s) I converted earlier
found an error in the console alerting to the fact that each row should have a unique key, so I added keys one concern: the upcoming notifications keys end up being the dates - could those ever be repetitive?
took out the code that was a part of a different effort to prevent confusion, also removed commented out code form replacing elements
the "Upcoming Notifications" and "Dummy Notification in 5 Seconds statements were hard coded, I've added them as internationalized strings and updated the en.json file
I started to convert the data storage, data is fine in general-settings.js but is failing to make it through main-control.html to ControlDataTable.jsx
this is where I'm struggling with the parameterization of i18n keys
Added in code so the broken setting row is enabled, note the console.log statement demoing the different ways the info comes through
|
Next week, or whenever you resume this, you will probably want to figure out how to handle the popups that many of these When I ran into this issue on the Label screen, I tried using Menu, Dialog, and Modal from React Native Paper to no luck. These components are supposed to sit on top of everything else and actually get rendered higher up in the DOM tree. We won't be able to use these until the upper-level components (i.e. the entire Profile screen) are written in React. So I decided we can just keep using the existing popovers, which are done using Here are the relevant places in the code showing how I used
|
in Angular, string parameters need to be created as "'string'" - this should resolve some of the issues that I have been experiencing here Co-authored-by: Jack Greenlee <[email protected]>
after I learned how to pass strings, I was able to render the row as a list item, which will hopefully eventually be good for showing everything as a list, and can have icons built in Icons and passing through functionality are next
switching from plugin storage to scheduler storage Co-authored-by: Jack Greenlee <[email protected]>
after fix from Jack (2nd place to update where notifs come from) added in optional chain to reduce errors in table, took out diagnostic log statements, updated Logger.log to show num, time, and first notif
the setting-row process has been separated to More profile migrations e-mission#994
have a rough outline going of some setting rows that say and do the right thing! More complex actions, some text, and icons still need lots of work
I added a bunch of comments and whitespace to make it easier for myself to see what needs to be done -- I'll reformat once I have things working
most rows now migrated, still need to handle some with slightly unique functionality, styling with icons also needed!
all rows ugly but functional but need guidance on reminder time and demographics button
I am fine with leaving this (and anything else in the developer options) hardcoded for now. |
|
Great work!
It is probably because the OPCode you are using has already run its course. Make a new OPCode and see if notifications show up then.
Not important. It does work, just not when the app is in the foreground. |
|
A few other UI tasks for this:
|
reduced font size of titles to 14 and descriptions to 12, truncate descriptions at 4 lines, add optional descStyle and use it to make opcode monospace
because everything is now handled in ProfileSettings that is the only file that needs to be angularized.
centralized style for all "setting rows" to SettingRow
everything is now handled by ProfileSettings - so need to import react elements through general-settings!
there were timing issues in the notifications schedule leading to no or old notifications displayed. By leveraging promises in notifScheduler, now the update preferences returns a promise, which resolves when update is through (which resolves once notifications are fully scheduled) - the chain of promises allows the notifications to always be scheduled and up to date when they are retrieved in React
|
This branch is now stable, as far as I know. There was a little bit of refactoring required in order to have the notifications work properly - as documented by recent commits. Essentially, increased reliance on promises allowed for finer-grained control to ensure the notifications existed and were up to date before pulling them for display. Two notes of things I was unable to "fix"
Files in .jsx will need to move to .tsx eventually, but this PR is already lengthy and everything is stable as is. |
ensure that all notifs are loaded the first time (before any updates) by forcing update to return before proceeding with empty .then()
www/js/splash/notifScheduler.js
Outdated
| @@ -200,7 +250,7 @@ angular.module('emission.splash.notifscheduler', | |||
| return; | |||
| } | |||
| setUpActions(); | |||
| update(); | |||
| update.then(); | |||
There was a problem hiding this comment.
Can you add more explanation for what this does?
I read the commit message but still don't see why we don't just call update() here
There was a problem hiding this comment.
Just calling update() would mean that the "ready" method would return without waiting for the promise that update() returns to resolve. If we don't wait here, we have the same messiness as before with the notifications not being scheduled before we get them to populate the table
There was a problem hiding this comment.
If you change it, do you actually observe a difference? Even if you do, I don't think it's for the reason you think it is.
Adding a .then() block doesn't block the current scope from continuing. It attaches a function to be called asynchronously once the promise resolves.
The block inside ready().then(...) is going to finish executing whether you have a .then or not.
Secondly, none of the code here is determining when ready returns.
It's just for declaring the things you want to do after Ionic is ready and the app config is loaded.
There was a problem hiding this comment.
That makes sense, thank you for explaining! I think making that change must have been a lucky coincidence that the loading timed out right the few times I tried it then. Taking it out leaves a situation where sometimes it loads in the right timing and sometimes does not.
I think I'm going to have to find some condition to cause a refresh once that data is ready, kind of like the appConfig worked well for the sync and collect settings. I have refreshNotifications called on 'appConfig' now, but I think it's too soon since it sounds like that's the same time the ready in notifScheduler starts
There was a problem hiding this comment.
I ended up refactoring the way that getScheduledNotifs works so that if isScheduling (an existing boolean in NotificationScheduler) was true, it used scheduledPromise.then(*code to get notifications*) to ensure the scheduling was completed before the fetching. This works consistently.
It does feel a little clunky to me, and I'm a little worried that it might slow things down (though I haven't noticed that yet) so I'm definitely open to feedback on better ways to ensure the notifs are displayed consistently. I also think this is something that might get smoothed out once we're fully in React.
Not all configs have reminders, so the row to display and choose reminder times should only be shown if there are reminders enabled (if config.reminderSchemes exists)
the schedule appearing in devZone should also be depended on the existence of a reminder scheme
Particularly on initial startup, the notifications were not done scheduling before they were fetched and so they were often undefined or partial. When the `isScheduling` var is true, the get method now awaits resolution of the getScheduled promise before fetching. The fetch itself is awaited in the notificationsRefresh of ProfileSettings so that notifications are present before displaying them -> hoping this can be simplified upon full migration into React
after refactoring the way notifs are fetched, this method is no longer used
This update.then(), as @JGreenlee pointed out, doesn't do what I wanted it to, and so I'm putting it back how it was
… More-Profile-Migrations
Abby-Wheelis
changed the base branch from
framework_migration_prep
to
aria_and_fall_2023_rewrite
|
@JGreenlee have you reviewed the final version of this? I will take a look but I am not as familiar with the react codebase/migration patterns. So I can look at high level patterns, but not sure I can see if it is react-like. |
|
This PR is good and has been for a while. The only thing I'd comment on here are organizational / tidiness things, like the abundance of comments and console.log statements about the parts that are still "under construction" where this PR leaves off, but since #1009 extends on this PR, those things are being, and continue to be, cleaned up there. I think it would be confusing to make changes to this PR at this point, so I'd say we can merge this and anything that needs to be refined should just go in #1009 |
There was a problem hiding this comment.
I have read that the react style is to create components for everything, but it is still a bit of surprising to see how many there are when we get to our project's code 😄 We really do have a separate component for everything (or at least for anything that is not just a <SettingsRow>
I am fine with merging this as an intermediate step since the expanded rewrite is ongoing.
I've added some comments to think about as part of the expanded rewrite or future work.
Let's resolve them after they have been included in the expanded rewrite, or have had a new issue filed for tracking them.
As per issue e-mission#994, fixed the `Download JSON Dump`. The previous date-picker formatted the input as 'dd MMM yyyy', used luxon to format the object. This is a bit hacky - going to rewrite the 'ControlHelper' service, and completely remove the `moment()` altogether.
I've started a new branch on my end, to try and separate the data table conversions, which are almost good to go, from other conversions on the profile page, which are presenting more trouble than I expected.
What I am having an issue with right now, is passing the i18n keys from the markup in general-settings.html into the react component defined in SettingRow.jsx - the key is currently coming through as the number 0