🧹 Post-Migration Code Cleanup #1124

JGreenlee · 2024-01-26T20:22:51Z

Summary of this PR

Code formatting

unify function declarations: const vs function

(initiative previously described in 0527569)

To be more consistent with the rest of the codebase, as a rule of thumb we're having top-level functions use 'function' synax while one-liners or nested functions are arrow functions.

Named functions that are at the top-level of a file, React component, or custom hook, should be declared as function foo() { ... }.
Anonymous / inline functions, and one-liner functions, will be declared as const bar = () = { ... };

These general rules should help to keep some consistency throughout the codebase.

use 'const' and 'let' instead of 'var'

in ES6, 'const' and 'let' are preferred over 'var', which is sort of outdated now.
In a few places 'var' was still used. For best practice we replace these.

cleanup all console/log statements

-- Replace all uses of console.log or console.debug with the functions from logger.ts (logDebug, logWarn, displayError). This ensures that all log statements are recorded by both the Javascript console AND the Cordova Logger plugin
-- Format all these log statements concisely and in a unified way (longer statements make use of `template strings` with interpolation, and the statements are put on as few lines as possible to mimimize clutter)
-- Remove log statements that are deemed redundant or excessive (we want to be thorough but not spammy - avoid log statements that run on every single re-render)

Addressed review from #1106

Implemented an `AlertManager`

add AlertManager for global alerts

There's a bunch of places in the app where we need to show a SnackBar or "toast"-style message.
But to do this, we've been having to keep each SnackBar's visibility as state so it can show/hide depeding on its "visible" value.
It would be simpler to be able to do this from anywhere in the codebase, though, not just by setting local state.
So I refactored AlertBar to behave as a global stack of alert messages. AlertBar is instantiated once at the top level in App.tsx. A static class called AlertManager can be used to add messages to this stack from anywhere in the codebase.
As a result, components no longer need to have their own instance of AlertBar and they don't have to control the visibility themselves.
They just call AlertManager.addMessage when they want to show a message.

UI and accessibility changes

Before	After
Before	After
Before	After

(initiative previously described in e-mission@0527569) >To be more consistent with the rest of the codebase, as a rule of thumb we're having top-level functions use 'function' synax while one-liners or nested functions are arrow functions. Named functions that are at the top-level of a file, React component, or custom hook, should be declared as `function foo() { ... }`. Anonymous / inline functions, and one-liner functions, will be declared as `const bar = () = { ... };` These general rules should help to keep some consistency throughout the codebase.

in ES6, 'const' and 'let' are preferred over 'var', which is sort of outdated now. In a few places 'var' was still used. For best practice we replace these.

-- Replace all uses of console.log or console.debug with the functions from logger.ts (logDebug, logWarn, displayError). This ensures that all log statements are recorded by both the Javascript console AND the Cordova Logger plugin -- Format all these log statements concisely and in a unified way (longer statements make use of `template strings` with interpolation, and the statements are put on as few lines as possible to mimimize clutter) -- Remove log statements that are deemed redundant or excessive (we want to be thorough but not spammy - avoid log statements that run on every single re-render)

…into code-cleanup-jan2024

This was added in a relatively recent version of i18next; some online resources haven't updated to reflect it https://www.i18next.com/overview/api#resolvedlanguage `language` is what i18next detected or we have set it to. It could be either a locale code like 'en-US' or a plain language like 'en'. `resolvedLanguage` is what language is actually being used for translation, after considering what is detected, what langauges we provided, and what fallbacks we specified. In our case this should always be either 'en', 'es', or 'lo' - the 3 languages we support at this time. So `resolvedLanguage` is the safe property to use and we should avoid `language`.

e-mission#1106 (comment)

Although the 'ch' unit works here, is not officially a valid type for React Native Web Changing to `25` to match the Label screen DateSelect

The `Icon` component was added in 5.11.0 and is referenced in CustomLabelSettingRow. So we must use 5.11.0 at minimum

Reach Native Paper didn't have a standalone <Icon> component until recently. Now that it does, we can get rid of the component we had made and just use theirs.

If `getConfig` resolves to null (meaning there is no stored config & we are on the Welcome page), then we should not call Object.keys on it. (Object.keys expects a non-null input) (There was causing an error in the console, but not a crash)

the textStyle of the datatable cell was not getting applied to the underlying Text element. With style applied on the Text component directly, the label is 12pt and bold

It is actually expected for appConfig to be initially null here, and we don't need to show an error message for it. EnketoModal is not rendered until it is needed; it hasn't called useAppConfig() in advance. So until useAppConfig() resolves, appConfig is null. Once it does resolve and appConfig is set, this useEffect runs again and initSurvey() gets called.

We avoid hardcoding colors to make dynamic theming easier

The initial state of modesShown should be 'labeled' only if the trip has a label; otherwise it should be 'detected' (since there will be no toggling) With the initial state being 'detected', if there are multiple modes then the OverallTripDescriptives will show

The declaration of the Appbar.Header is repeated across every screen in the UI. Let's unify all these declarations into one component called <NavBar>. We already had a component called <NavBarButton>. It makes sense to put both in the same file since they will always be used together. So the new filename will be NavBar.tsx and it will export both <NavBar> and <NavBarButton>.

We had the NavBar height set at 46px to match the old AngularJS + Ionic UI. It was a bit cramped. The migration is done and we no longer have to worry about that, so what's the optimal height? Well for accessibility reasons, click zones of the action items in the navbar should ideally be at least 44x44. If the NavBarButtons are going to be 44px tall, the NavBar should be a bit taller than that. 56px looks comfortable.

There's a bunch of places in the app where we need to show a SnackBar or "toast"-style message. But to do this, we've been having to keep each SnackBar's visibility as state so it can show/hide depeding on its "visible" value. It would be simpler to be able to do this from anywhere in the codebase, though, not just by setting local state. So I refactored AlertBar to behave as a global stack of alert messages. AlertBar is instantiated once at the top level in App.tsx. A static class called AlertManager can be used to add messages to this stack from anywhere in the codebase. As a result, components no longer need to have their own instance of AlertBar and they don't have to control the visibility themselves. They just call AlertManager.addMessage when they want to show a message.

This comma didn't seem to have any adverse effects, but it doesn't belong here

Since AlertBar / AlertManager can now be used from anywhere in the codebase, they are not specific to the Profile screen and can be moved to a more general location

Apparently, Prettier actually does want a comma here because this is an argument to a function, and we have trailing commas enabled. It just looks strange because it's following a JSX element

'remote notifications are not supported in the simulator' - we expect this error in the Simulator, but don't want to completely silence it so that people don't expect it to work. An Alert works for this

On the other screens of the UI, we use a NavBarButton for clickable buttons with text. I realized the Log Out button is inconsistent with this, so I changed it to a NavBarButton. I tested this with a screen reader and it is still accessible (since it uses the Button component it is grouped as and recognized as 1 element with role 'button') Using a NavBarButton made the log out icon smaller than before, so while making this change I revised the NavBarButton props. I added an optional "iconSize" that can be used to override the default 20. I removed "onPressAction" because the Button component already has "onPress" which we can just use. I organized the styles for both NavBar and NavBarButton, and made them able to be extended on by props.

https://github.com/e-mission/e-mission-phone/pull/1106/files#r1473736741

As it turns out, Enketo supports a more mobile-friendly forms interface than we've been using. I noticed the mobile-friendly interface whle using Kobotoolbox, and I wondered why it didn't show up the same in e-mission-phone. I dug into Enketo source code and found that it automatically detects mobile devices by inspecting the user agent and platform info. (e.g. if it contains "iPhone", then mobile=true). It applies "touch" class to the <html> element to flag this. This is indeed observed in e-mission-phone, but because we only inject Enketo styles into the views they open in (i.e. we scope those styles under .enketo-plugin in styles), the <html> element's "touch" class has no effect. An easy solution is to just add the "touch" class at the root of our enketo content (for our purposes, we can assume e-mission-phone will always be on a phone / tablet a since it's a mobile app. We don't really need the 'mobile detection' that Enketo does) The result of this is that the enketo forms have larger click areas on multiple-choice / multi-select questions, which is better for accessibility.

JGreenlee · 2024-02-20T20:24:53Z

Summarized the changes in the above description.

the log statement in getNotDeletedCandidates was changed from a console.log to a logDebug in 0a8ed76 The associated test checks the log statements; it was still watching for console.log and needed to be updated to logDebug

in 31d472e, the reader.onprogress, reader.onerror, and reader.onload functions here were changed from regular `function` declarations to arrow functions. Usually this makes no difference in the operation of the code, but there is one exception. Arrow functions don't get their own `this`, they just inherit it from the scope. When I swapped this out and made it an arrow function, I didn't notice it used `this`. In this instance, `this` was the same thing as `reader`, which we already have a reference to. So it's an easy swap.

codecov · 2024-02-21T01:21:33Z

Codecov Report

Attention: Patch coverage is 82.25806% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 77.49%. Comparing base (3fbc6b5) to head (14f72de).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1124      +/-   ##
==========================================
- Coverage   78.37%   77.49%   -0.89%     
==========================================
  Files          28       29       +1     
  Lines        1702     1693       -9     
  Branches      366      370       +4     
==========================================
- Hits         1334     1312      -22     
- Misses        368      381      +13

Flag	Coverage Δ
unit	`77.49% <82.25%> (-0.89%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
www/js/config/useImperialConfig.ts	`51.85% <100.00%> (ø)`
www/js/diary/diaryHelper.ts	`73.46% <ø> (ø)`
www/js/i18nextInit.ts	`84.61% <100.00%> (ø)`
www/js/metrics/customMetricsHelper.ts	`95.91% <ø> (ø)`
www/js/onboarding/onboardingHelper.ts	`43.24% <100.00%> (ø)`
www/js/plugin/clientStats.ts	`90.62% <100.00%> (+0.96%)`	⬆️
www/js/plugin/logger.ts	`93.75% <100.00%> (ø)`
www/js/splash/customURL.ts	`100.00% <100.00%> (ø)`
www/js/splash/notifScheduler.ts	`98.37% <100.00%> (-0.10%)`	⬇️
www/js/splash/startprefs.ts	`95.74% <100.00%> (-0.41%)`	⬇️
... and 12 more

Instead of being purple (which is the default RN Paper theme color), AlertBars will now match the rest of the UI and use blue / blue undertones

'msgKey' is for translation keys, which 'success -> ' is not. We can just use 'text'. This is not user facing and does not necessarily need to be translated anyway

Since Enketo's themes use SCSS it isn't that hard to override the orange color. This makes it a bit more cohesive with the rest of the app.

These buttons aren't part of Enketo; they are provided by EnketoModal. That means we can customize them. I made the "next" and "save" buttons at the bottom of Enketo surveys into RN Paper <Button> components, matching the rest of the UI and making the survey view more cohesive within the context of app. Also, these buttons (as well as the "Return to beginning" and "Go to end" buttons) now have icons alongside the text.

shankari · 2024-03-04T15:57:13Z

@JGreenlee thanks so much for finishing this up! I will work on reviewing the code today. In parallel, it would be great if you could resolve the merge conflicts from simpler/higher priority changes that I merged earlier.

JGreenlee · 2024-03-04T17:57:57Z

Resolved merge conflicts

shankari

Great fixes - the code is significantly cleaner now!
I am going to merge this now so the other PRs can be rebased to include it, but do have some questions from the review.

shankari · 2024-03-15T06:23:03Z

www/js/components/AlertBar.tsx

I am not going to hold up the release for this, but it looks like we need to add some additional tests for this functionality. For example, there is no test for setListener or addMessage (or that is what `codecov tells me)

shankari · 2024-03-15T06:34:32Z

www/js/control/ControlSyncHelper.tsx


 //forceSync and endForceSync SettingRows & their actions
 export const ForceSyncRow = ({ getState }) => {
  const { t } = useTranslation();
  const { colors } = useTheme();

  const [dataPendingVis, setDataPendingVis] = useState(false);
-  const [dataPushedVis, setDataPushedVis] = useState(false);


This is fine for now, but we may want to resurrect it when we revisit data sync from the phone to the server

Behavior is the same anyway; this was replaced by a call to AlertManager

shankari · 2024-03-16T03:30:06Z

www/js/diary/list/DateSelect.tsx

-    if (!tsRange.oldestTs) return;
+    if (!pipelineRange || !tsRange.oldestTs) return;


future: why do you need this?

It's really just so Typescript knows pipelineRange isn't null and doesn't give a warning.

I don't think there's actually a situation where tsRange could be set if pipelineRange was null. But i had to add this check so the type definitions all check out with each other

shankari · 2024-03-16T04:24:00Z

www/js/plugin/logger.ts

-  window['Logger'].log(window['Logger'].LEVEL_DEBUG, message);
+  window['Logger']?.log(window['Logger'].LEVEL_DEBUG, message);


It is obviously a good thing to check this for robustness, but is this just to be on the safe side, or has this ever been null? As long as we wait for all plugins to be initialized before continuing initialization, window['Logger'] will always exist, right?

We do wait for deviceready before proceeding with the main initialization, but there a little bit of initialization that happens outside of that (specifically, the i18next initialization which does not depend on Cordova). I have noticed window['Logger'] being undefined during that.
This could be reworked in necessary.

It would also happen if there was a stray log statement at the root level of any file (not within a function body), since code at the root-level always gets executed immediately.

shankari · 2024-03-16T04:24:59Z

www/js/plugin/storage.ts

-  console.log('STORAGE_PLUGIN: Called syncAllWebAndNativeValues ');
+  logDebug('STORAGE_PLUGIN: Called syncAllWebAndNativeValues');


lots of codecov errors here, is this function actually tested now?

TODO potential regression
Need to look into this. This function storageSyncLocalAndNative doesn't appear to be called anywhere.

getUnifiedValue is working and is validated by testing. But storageSyncLocalAndNative is not.

This would suggest we are only syncing values for a particular key when it is being requested.
We aren't regularly syncing all keys on each app startup / resume, as was the pre-migration behavior

shankari · 2024-03-16T04:28:43Z

www/js/splash/customURL.ts

-  callback: (url: string, urlComponents: UrlComponents) => void,
-) => void;
-
-export const onLaunchCustomURL: OnLaunchCustomURL = (rawUrl, handler) => {
+  handler: (url: string, urlComponents: UrlComponents) => void,
+) {


whoa! handler -> callback was a pretty bad regression. How did we miss it?

It was not a regression. just a bit confusing.

The type of the function (OnLaunchCustomURL) was declared separately from the function itself (onLaunchCustomURL).
The name of the second positional argument in the type definition was callback, while in the function body, it was handler.

In the rest of the codebase we don't type functions in this way, so to match style I moved the types of the arguments to be directly in the function. Which meant there is only one declaration and no confusing difference in argument names

shankari · 2024-03-16T04:48:25Z

www/js/survey/enketo/AddedNotesList.tsx

-      additionEntries.splice(index, 1);
+      // if entry was found in additionEntries, remove it
+      if (index > -1) {
+        additionEntries.splice(index, 1);
+      }


this was changed as part of f9c3c57, which refers to https://github.com/e-mission/e-mission-phone/pull/1106/files#r1473736741 which only talks about logging.

I don't see anything wrong with the fix, just that the motivation is not clearly outlined.

While I was adding the first guard clause and the log statement, I realized a second guard clause would be safer here to handle the case where delete requests overlap and the entry being deleted no longer exists (like the button was double-tapped or something)

shankari · 2024-03-16T04:54:54Z

www/js/survey/enketo/EnketoModal.tsx

-    if (!appConfig) return console.error('App config not loaded yet');
+    if (!rest.visible || !appConfig) return;


Don't we want to log the error in this case? it is unlikely, but that is what will make it hard to figure out if it ever happens in the field. And we won't be able to reproduce it. So this log, if it exists, will hopefully save the day!

TODO add log back

JGreenlee added 3 commits January 26, 2024 00:09

use 'const' and 'let' instead of 'var'

adc6620

in ES6, 'const' and 'let' are preferred over 'var', which is sort of outdated now. In a few places 'var' was still used. For best practice we replace these.

JGreenlee changed the base branch from service_rewrite_2023 to master February 12, 2024 15:55

JGreenlee added 4 commits February 12, 2024 11:16

Merge branch 'master' of https://github.com/e-mission/e-mission-phone …

03fc69a

…into code-cleanup-jan2024

use PascalCase for all type defs

a26d039

use LocalDt in MetricsTypes

069a72e

JGreenlee mentioned this pull request Feb 12, 2024

🎀 UI Migration Wrap-up #1106

Merged

JGreenlee added 21 commits February 12, 2024 11:39

LabelTab: guard clause for pipelineRange, both start_ts and end_ts

125d009

use Boolean() instead of !!

2453e9a

e-mission#1106 (comment)

use AppConfig type def on the useAppConfig hook and in inputMatcher

c483322

remove 'ch' unit from MetricsDateSelect

a4eb833

Although the 'ch' unit works here, is not officially a valid type for React Native Web Changing to `25` to match the Label screen DateSelect

bump 'react-native-paper' to ^5.11.0

260f213

The `Icon` component was added in 5.11.0 and is referenced in CustomLabelSettingRow. So we must use 5.11.0 at minimum

use the new Icon component from RN Paper 5.11

1ed9e57

Reach Native Paper didn't have a standalone <Icon> component until recently. Now that it does, we can get rid of the component we had made and just use theirs.

in useAppConfig, check config contents safely

a717560

If `getConfig` resolves to null (meaning there is no stored config & we are on the Welcome page), then we should not call Object.keys on it. (Object.keys expects a non-null input) (There was causing an error in the console, but not a crash)

AddedNotesList: adjust UI

ed49146

the textStyle of the datatable cell was not getting applied to the underlying Text element. With style applied on the Text component directly, the label is 12pt and bold

use theme colors for logout button

8101388

We avoid hardcoding colors to make dynamic theming easier

remove weird comma

76f4d5d

This comma didn't seem to have any adverse effects, but it doesn't belong here

move AlertBar to /components

c6a9777

Since AlertBar / AlertManager can now be used from anywhere in the codebase, they are not specific to the Profile screen and can be moved to a more general location

revert "remove weird comma"

9592107

Apparently, Prettier actually does want a comma here because this is an argument to a function, and we have trailing commas enabled. It just looks strange because it's following a JSX element

make push notifications warning an AlertBar

b573e06

'remote notifications are not supported in the simulator' - we expect this error in the Simulator, but don't want to completely silence it so that people don't expect it to work. An Alert works for this

explanation comment in dynamicStyleSheet.d.ts

5f8bb77

AddedNotesList: safer guard clauses in deleteEntry

f9c3c57

https://github.com/e-mission/e-mission-phone/pull/1106/files#r1473736741

JGreenlee marked this pull request as ready for review February 20, 2024 20:24

JGreenlee added 2 commits February 20, 2024 20:06

fix inputMatcher logging test

9174545

the log statement in getNotDeletedCandidates was changed from a console.log to a logDebug in 0a8ed76 The associated test checks the log statements; it was still watching for console.log and needed to be updated to logDebug

add "inverse" colors to appTheme for AlertBars

0ce1e7d

Instead of being purple (which is the default RN Paper theme color), AlertBars will now match the rest of the UI and use blue / blue undertones

JGreenlee marked this pull request as draft February 21, 2024 03:23

JGreenlee added 3 commits February 20, 2024 23:24

finish adding typings for LabelTabContext

658f00f

simplify alert creation in ProfileSettings

0e50c8b

'msgKey' is for translation keys, which 'success -> ' is not. We can just use 'text'. This is not user facing and does not necessarily need to be translated anyway

SettingRow: remove unused variable 'descriptionText'

2796702

JGreenlee marked this pull request as ready for review February 27, 2024 20:47

JGreenlee added 2 commits February 27, 2024 21:10

use blue instead of orange for enketo surveys

b62861e

Since Enketo's themes use SCSS it isn't that hard to override the orange color. This makes it a bit more cohesive with the rest of the app.

Merge branch 'master' into code-cleanup-jan2024

14f72de

shankari approved these changes Mar 16, 2024

View reviewed changes

shankari merged commit 5812eee into e-mission:master Mar 16, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧹 Post-Migration Code Cleanup #1124

🧹 Post-Migration Code Cleanup #1124

JGreenlee commented Jan 26, 2024 •

edited

Loading

JGreenlee commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 21, 2024 •

edited

Loading

shankari commented Mar 4, 2024

JGreenlee commented Mar 4, 2024

shankari left a comment

shankari Mar 15, 2024

JGreenlee Mar 18, 2024

shankari Mar 15, 2024

JGreenlee Mar 18, 2024

shankari Mar 16, 2024

JGreenlee Mar 18, 2024

shankari Mar 16, 2024

JGreenlee Mar 18, 2024

shankari Mar 16, 2024

JGreenlee Mar 18, 2024

shankari Mar 16, 2024

JGreenlee Mar 18, 2024 •

edited

Loading

shankari Mar 16, 2024

JGreenlee Mar 18, 2024

shankari Mar 16, 2024

JGreenlee Mar 18, 2024

		if (!tsRange.oldestTs) return;
		if (!pipelineRange \|\| !tsRange.oldestTs) return;

		window['Logger'].log(window['Logger'].LEVEL_DEBUG, message);
		window['Logger']?.log(window['Logger'].LEVEL_DEBUG, message);

		console.log('STORAGE_PLUGIN: Called syncAllWebAndNativeValues ');
		logDebug('STORAGE_PLUGIN: Called syncAllWebAndNativeValues');

		if (!appConfig) return console.error('App config not loaded yet');
		if (!rest.visible \|\| !appConfig) return;

🧹 Post-Migration Code Cleanup #1124

🧹 Post-Migration Code Cleanup #1124

Conversation

JGreenlee commented Jan 26, 2024 • edited Loading

Summary of this PR

Code formatting

Addressed review from #1106

Implemented an AlertManager

UI and accessibility changes

JGreenlee commented Feb 20, 2024 • edited Loading

codecov bot commented Feb 21, 2024 • edited Loading

Codecov Report

shankari commented Mar 4, 2024

JGreenlee commented Mar 4, 2024

shankari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGreenlee Mar 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGreenlee commented Jan 26, 2024 •

edited

Loading

Implemented an `AlertManager`

JGreenlee commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 21, 2024 •

edited

Loading

JGreenlee Mar 18, 2024 •

edited

Loading