🚀 Framework migration prep has just been submitted to production #1015
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We rely heavily on Angular for internationalization, both directly in the HTML and in the JS behind the scenes. However, it's specific to Angular and we can't use it in React components. i18next is compatible with React / React Native Web and we will be able to use it in both frameworks at the same time. The i18n JSON files themselves can keep the same exact structure. Only a few strings were changed (the 'equals X cookies, equals X ice cream', etc) - i18next has a different mechanism for pluralization and we don't have to use ICU for only 3 strings.
I had removed 'recent.js' because I thought all of these controls were unmaintained and broken. "Check Log" and "Check Sensed Data" seem good enough to keep though, so I'm bringing them back.
🚀 Prep Tasks for Framework Migration
#974 (comment) These files represent settings for various plugins; housed in their respective repos, not here. They should not have been committed. I added them to the .gitignore so that they are not accidentally committed in the future. I also removed the `transition-notify` files from the script, since it is not being used anymore.
Without `type="text/javascript"`, the devapp would not poll for autorefresh
(I think) cards used to expand when this button was clicked. Now, cards just automatically grow to the right height and this button does nothing. Let's remove it, and then add some left margin to the remaining buttons so they don't look out of place.
https://stackoverflow.com/questions/71668256/deprecation-notice-reactdom-render-is-no-longer-supported-in-react-18 React 18 uses `createRoot` instead of `render`. To be safe let's use the new recommended method. It works the same, but now we won't have to look at the annoying deprecation notice.
This was inserted here as a proof-of-concept. We'll keep the file as an example, but remove it from being rendered anywhere. #974 (comment)
This was only used for consistent map settings. This is all handled in LeafletView.jsx now. I copied the old tileLayerOptions to LeafletView.jsx, for consistency - but I removed `reuseTiles` because it was removed in Leaflet v1.0
this was removed by mistake #974 (comment)
We'll be getting modes from the server in the string form "MotionTypes.WALKING" instead of integer values of the enum.
So, we'll reorganize MotionTypes and create `motionTypeOf()`, which looks up the appropriate MotionType by key ('WALKING') given the string input ('MotionTypes.WALKING').
#974 (comment) Using $broadcast for 'recomputeListEntries' and 'scrollResize' does work. We just needed to include `$rootScope` in the controller and factory definitions. Now we can remove another use of jQuery because we don't need `getScrollElement()`. Instead of calling `.trigger('scroll-resize')`, we'll broadcast to `infinite_scroll_list`, where the scrollview will be updated using `$ionicScrollDelegate.resize()`. This seems like a less hacky way of getting the cards to resize and recompute.
#974 (comment) We will remove 'location', and comment out `stop` in case we ever want to show stops as their own points.
The filter text is translated in the JS and does not need to be translated here. We get warnings from i18next if we try to translate already-translated text.
Prior to this change, the map would grow too large. We want it to only get to 50% of the parent. The CSS changes here relate to how React Native Web is injecting into the existing DOM structure. It uses a bunch of wrapper divs (of class names starting with 'css-view') and we want to make sure these are getting as big as, but only as big as, the containers they are injecting into.
Draft trip cards have a muted green look and subtle grid lines in the background. Those cards' details pages should also reflect this style. And in the CSS, I reformatted the `linear-gradient` statements so that they are less verbose.
With the `collection-repeat` mechanism of Ionic, the infinite scroll list reuses DOM elements as you scroll. In the case of a large list, this helps to prevent the DOM from getting too big. However this didn't play nice with our React helpers; we would get the following warning: "You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before." The solution here is to have a Map which keeps track of which DOM elements for which roots have already been created. This way we don't duplicate any roots. A Map was chosen rather than an object because the key needs to be an object reference (in this case a DOM node).
not to be confused with $scope.data.allTrips, $scope.allTrips was not actually used anywhere. Removing it did not change any observable behavior.
For the old solution (2b72468), we inserted a hack into the Leaflet code. The new solution keeps track of the maps that have been created, and supplies a function `invalidateMaps()` which will call `invalidateSize()` on every map. This is called in infscroll, `recomputeListEntries()`.
There are countless QR code generation libraries; I chose this one because it works using SVGs rather than <canvas>, which means it will still work when we switch to native rendering and can no longer use HTML. Because we have to `angularize` components if they are going to be used directly in AngularJS, I created a wrapper around QRCode (www/js/control/QrCode.jsx). Once the parent components have been converted to React, and the QrCode component no longer needs to be angularized, we can remove the wrapper and just use QRCode directly. -I also generally cleaned up `saveTokenFile.html` a bit; removed <center> tags and fixed indentation
This is better practice to load our scripts after body. Some scripts might be looking for the body element / its contents - and if that script is in the head it can't access yet. The Cordova script can stay in the head.
In plain HTML, <style> tags work globally. This means a style declared in a child can affect its parents, siblings, and even distant cousins. This is problematic now that we are programatically setting the colors. For draft trips, because they have a 'flavor' different than the main app theme, their <style> tags contain different colors than non-draft trips. But the draft trip <style> tags were still being overridden by the other <style> tags because they are all global. To remedy this we must prefix those CSS selectors with an ID specific to the current map. This way the styles are scoped to the map currently being defined.
the other gray background clashed a bit with the blue-tinted cards. This gray is a nearby color to the blue scheme already in use For the label screen it is controlled appTheme and applied in LabelListScreen. Since the profile and dashboard are not converted, the same color is applied to the "pane" class in style.css
in both the survey tab and the options tab the column labels needed to be updated to fix the survey, which was broken for Lao - both now read "label::ລາວ (lo)"
new survey has the correct column names so it will work in Lao
in both the survey tab and the options tab the column labels needed to be updated to fix the survey, which was broken for Lao - both now read "label::ລາວ (lo)"
this file has been re-uploaded as "demo-survey-v2.xlsx"
in both the survey tab and the options tab the column labels needed to be updated to fix the survey, which was broken for Lao - the names in this file were inconsistent and causing only the questions or the answers to show
this file has the proper column name "label::ລາວ (lo)" for the Lao options and survey
Jul 2023 fixes - related to label screen react migrations
We store all the surveys in `survey_resources` but we read them from `www/json` in the real app. Copying the file over to the correct location This is the final fix to get the survey to work #1005 (comment)
While completing the onboarding / 'intro', the Save OPCode slide would be skipped. The next slide, which is the demographics survey, would load after ~1 second and then overtake the Save OPCode slide. I am guessing that when you move to the next slide of an ion-slide, it looks ahead to the next slide and creates it ahead of time. By systematically commenting out code, I isolated the issue and determined it only happens when the Enketo Form object is `init()` (service.js -> _loadForm). I think there is some code there that focuses the form and ensures the form is visible. So with it being off-screen at the time it is initialized we get strange results. So the workaround here is to prevent the survey from being initialized before that slide is active. intro has a slideIndex we can look at, and we will only start rendering the contents of the survey slide once slideIndex reaches 4 (the fifth slide / the survey). I also found that in EnketoDemographicsInlineCtrl, the 10-second $timeout that waits for the slide to be in place is too low. 50 is safer. Overall, this is a pretty temporary solution; the intro will be rewritted in React before too long. We will use something other than ion-slide, and very likely redesign the onboarding process in general.
The "No Travel" banner is supposed to show when there are no trips. However, it was showing for a couple seconds, after app launch, but before the pipelineRange is fetched.
This is a place where we really want the spinner to show.
In LabelTab, if nothing had been processed for a user yet, getUnprocessedLabels would come back with a pipelineRange of {start_ts: null and end_ts: null}, but we would ignore this result.
We should setPipelineRange to this even if it's empty so that we can indiate it's already been fetched.
Then in TimelineScrollList, instead of checking if pipelineRange isn't defined, we should instead check if it *has* been set, but the end_ts is null. In this case we show the banner.
This way, if pipelineRange itself is not set yet, we get the spinner.
Also added comments to better understand the conditional logic for what's shown in the timeline scroll list.
I thought that surfaceVariant would be the appropriate color to tweak and use as the background. But there is a color from the RN Paper theme called 'background' which is more appropriate. I noticed that 'surfaceVariant' is used for DataTable, and I think some Dialogs, so changing it for the background has some side effects. But 'background', as far as I can tell, only affects the background of the LabelScreen. I tweaked the color in the process and also noted where surfaceVariant is used.
On languages with long button labels, we can just have overflows truncate with '...' instead of trying to cram multiple lines of text onto 1 small button.
Error was thrown in AddNoteButton -> getPrefillTimes because unprocessed/draft trips did not have `timezone` defined in their local_dt objects. In services.js we compute the properties for unprocessed trips in `points2TripProps`. To get the dt objects we call `moment2localdate`, which takes a tz argument, but we did not use it. We have access to the timezone names from `startPoint` and `endPoint` metadata, so let's use them. Now activities can be added to draft trips, and the prefilled timestamps are correct. Tested trips from various different timezones just to be sure it was not reverting to my local timezone.
both qrcodes (that in the login process and than in settings) should be generated from the same string, so the image is the same, that is now the case on production version, the "emission" prefix will be swapped out for "nrelopenpath"
I found another mistake in the column names, the hint column was different than the rest of the Lao columns, so it was reading as 2 Lao options (one for questions and answers and one for hints) this should fix but needs a json conversion!
Converted and uploaded json, and renamed xml because it was named incorrectly
I found 4 hardcodes strings in the login process while testing the app in Lao - those are internationalized here and pairs with e-mission-translate PR#36 e-mission/e-mission-translate@390786f
…mission-phone into more-jul-2023-fixes
🐛 More Jul 2023 Fixes
Resolves a bug tracked in e-mission/e-mission-docs#948 The pipeline range should only be fetched when the label screen is initialized or refreshed. When a trip is labeled, we should not need to fetch the pipeline range - this was happening because the old `getUnprocessedLabels` function called `getPipelineRangeTs` every time it was called. This function was rewritten into `getUnprocessedInputs` in `timelineHelper.ts`. It will accept the pipelineRange as an argument. Now in LabelTab, only on init / refresh will we request the pipelineRange. Subsequent calls to `getUnprocessedLabels` will use the already-fetched pipelineRange. We will also handle errors better - if retrieving pipeline range fails (network error), we should show error popup and stop loading spinner. Testing done: -Labeled several trips - all labels appeared within ~1 second. -Disconnected network, tried labeling trips - all labels appeared within ~1 second. -While network disconnected, tried refreshing label screen - popup "Error while loading pipeline range". Same result on app launch while network disconnected. extra tidying note: - removed getAngularService("DiaryHelper") from LabelTab because it no longer used (the functions that were previously used there had already been moved to the new Typescript file diaryHelper.ts)
The last commit handles network errors better - we show a popup error if the pipeline range can't be fetched.
In that situation, we don't want to render anything at all for the TimelineScrollList. `listEntries` is null so there's no cards to show, and we shouldn't show the header/footer ("Load more") buttons either. Let's modify this condition to only show the FlashList if `listEntries` is defined.
#1013 (comment) Unprocessed inputs can be either: > in the phone cache and not pushed up > OR pushed up and in the server cache, but not yet processed We need a way to refresh the local unprocessed inputs without checking the server. To do this we split the getUnprocessedInputs function into 2: getAllUnprocessedInputs will used the UnifiedDataLoader to retrieve and combine both local and remote inputs, while getLocalUnprocessedInputs will only check the (local) usercache without making server calls. In LabelTab, we'll getAllUnprocessedInputs on init/ on refresh, but when a trip is labeled we will only getLocalUnprocessedInputs because we don't need to check the server again each time.
🐛 Fix extra pipeline range request every time a trip is labeled + handle network error better
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So we now merge to "master" and future changes are bugfixes
Related PRs: