🚀 Prep Tasks for Framework Migration #974
Merged
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
--The colors and icons for modes have been condensed into one dictionary MotionTypes, instead of having multiple verbose lookup functions --From the server, we'll receive 'sections' of composite trips. We'll sort the received 'locations' into each section so that we can draw them sections as distinct LineStrings on the map. --Since we are doing GeoJSON conversion on the phone now, the GeoJSON representation of a trip will be localized to the 'geojson' property of the trip. (ie. `trip.geojson`). This keeps the trip object itself uncluttered.
-When a date is chosen, trips from that week will be loaded into the view (3 days before date to 3 days after date). -Downloading more trips still happens on a weekly basis, but can be done in either direction now - past or present. --So if the date picker is used to pick a past date, we can then load the week after. -To download more trips, there are new buttons at the beginning and end of the list. So, the list consists of a 'header', any trips/place/untracked, and a 'footer' --'displayTimelineEntries' was renamed to 'listEntries' to make this more logical -The filters <select> and the date <input> will now track and display 1) the number of trips shown / total number and 2) the date range that is being displayed
-We no longer need 'tripgj' in 'infinite_scroll_detail', since the trip object structure is now unified and will include all the detail we need -I decided the 'sectionList' only needs to show if `trip.sections.length > 1`. If the trip is only one section, all the info is redundant and doesn't need to be shown again. -The logic to create the chart was copied from 'detail.js', except instead of iterating the points of each section, we iterate the composite trip 'locations' (which were downsampled on the server to <=100 locations) --This way, the composite trip object won't have to include the lengthy 'speeds' and 'distances' arrays in each section (probably best for bandwidth)
These buttons were not looking/behaving consistently between android and iOS. For the pseudoelements to work uniformly, let's put a wrapper around the <select> and <input>. Then tweak styles. -Also, remove the dash between start and end date, as there isn't room for 3 lines of text on most devices. Instead, position an <hr> in the middle for a separator line.
Instead of a hacky jQuery solution, we can use the 'translate' filter to interpolate in the number of trips shown by the filter, and the total number of trips. It's still a bit ugly, but at least it's more semantic. The english labels had (non-breaking-spaces) in them, which were not being parsed correctly. The other languages had all normal spaces, so I made the English spaces all normal too. This text is not at risk of being spread onto two lines.
-Make 'load more' buttons greyscale instead of blue. They don't need to be a focus of attention -Make 'start' and 'stop' time tags `width: max-content`. Before this change, on small screens, the tags would sometimes break down onto two lines - this ensures they stay as one line
- In most cases, when the date picker opens we should have it set on 'today' - but in case there haven't been any trips in a while it's logical to have the date picker start on where we left off. - And this way, when we're using test trips from 2016, we won't have to scroll waaaay back ;)
|
Here is a demo of the progress so far. 8mb.video-XT8-Kur57quk.mp4 |
This was referenced May 20, 2023
…mission-phone into unify-diary-label-screen
- The label screen now reads unprocessed trips at the same time as reading composite trips from the server. -- Unprocessed trips used to return in GeoJSON format. Now they return in a composite-trip-like format. -Draft trips will show in a muted, greenish color. Any references in the CSS to the blue 'accent' colors have been reworked to use CSS variables. (Some inline styles were adjusted as well.) This allows for more flexible theming of different UI components, and allows draft trips to become uniformly green.
|
Draft trips are showing. I believe the label screen now has all the desired core functionality.
|
JGreenlee
changed the base branch from
may_2023_point_release
to
master_for_platform
Unprocessed trips are no longer returned as GeoJson - they are now congruent with the 'composite trip' structure. So, it's misleading to call this 'trip2Geojson'.
|
After a high-level review of this, I will finally remove the diary screen and begin pruning all the unused code. |
We want draft trips to show in "To Label", so we will include `expectation`. We don't need `speeds` or `times`.
Rename card styles to be more descriptive - 'short' -> 'place' - 'greyed' -> 'untracked' Untracked time will now have a reddish-styled appearance, with a subtle x-grid in the background
LeafletView is the new React Web component that will replace ui-leaflet. This allows us to remove much of the Leaflet/maps logic from elsewhere and encapsulate it into one reusable component.
The maps embedded in trip cards should not have any scroll events. Let's just make everything inside .diary-map have pointer-events: none - that way the scroll events go to the card.
|
Ok so I'm going to say upfront that I am not going to be able to do a super-detailed review of this change because it is 243 files. I will do a high level pass, but we are finally at the point where there will be code in OpenPATH that I am not super familiar with. Yay! Thanks @JGreenlee |
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.
JGreenlee
changed the title
shankari
requested changes
May 28, 2023
There was a problem hiding this comment.
This is amazing progress. It looks like we have already finished most of the first two stages in
e-mission/e-mission-docs#857 (comment)
We haven't yet refactored everything into angular directives, but we can start doing that when the other two interns join.
I am finally done reviewing the changes and they look almost complete. Even the review took close to a workday to complete.
I have comments and suggested changes below, but they are not too onerous. I think we can push this to staging ~ May 31 and to production ~ June 7 so we get these improvements to users fairly quickly.
I will want to merge this to a separate branch to leave master_for_platform open for hotfixes while we test this; I have rebased accordingly.
I am not reviewing any of the CSS initially since we can always tweak it later.
shankari
changed the base branch from
master_for_platform
to
framework_migration_prep
|
I am actually tempted to merge this to the new branch as is so that the fixes go into separate PRs that are easier to review. |
|
Merging this to the new branch to make it easier to track changes for fixes based on the code review. |
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
e-mission#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.
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
This was inserted here as a proof-of-concept. We'll keep the file as an example, but remove it from being rendered anywhere. e-mission#974 (comment)
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
this was removed by mistake e-mission#974 (comment)
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
e-mission#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.
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
e-mission#974 (comment) We will remove 'location', and comment out `stop` in case we ever want to show stops as their own points.
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
JGreenlee
added a commit
to JGreenlee/e-mission-phone
that referenced
this pull request
May 30, 2023
shankari
added a commit
that referenced
this pull request
Jun 25, 2023
In #974 #980 and #990 @JGreenlee removed bower and added several dependencies to npm this commit copies those dependencies to the cordovabuild as well Testing done: - Ran npm install Added the following manual fix ``` diff --git a/www/index.js b/www/index.js index c2b2bb97..c7b6ae49 100644 --- a/www/index.js +++ b/www/index.js @@ -53,8 +53,10 @@ import './js/metrics.js'; import './js/control/general-settings.js'; import './js/control/emailService.js'; import './js/control/uploadService.js'; -import './js/control/collect-settings.js'; -import './js/control/sync-settings.js'; +import '../platforms/android/platform_www/js/control/collect-settings.js'; +import '../platforms/ios/platform_www/js/control/collect-settings.js'; +import '../platforms/android/platform_www/js/control/sync-settings.js'; +import '../platforms/ios/platform_www/js/control/sync-settings.js'; import './js/metrics-factory.js'; import './js/metrics-mappings.js'; import './js/plugin/logger.js'; ``` - `npx webpack` - `npx cordova build` Launches on both iOS and android with some minor issues e-mission/e-mission-docs#923
30 tasks
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.
This PR tackles the Prep tasks of the migration plan: (orange and yellow)
Diary-Label screen unification
Before removing the diary screen and relying solely on the label screen to display trips, there are a few features that had to be brought over to the label screen.
-- implements buttons to load more trips - next week / previous week
Pruning unused and unmaintained code
As you can see from the diff count, OpenPATH has accumulated a lot of 'dead code' over the years - from things that were added and removed, commented out, or just went stale. I sifted through the codebase and removed as much dead code as I could. I used the Code Coverage profiler in Safari devtools to help identify unused sections of JS, which saved a lot of time.
The main things that were removed:
No functionality is being lost; the goal is for only functional code to be preserved, and for only dead and nonfunctional code to be removed.
Dependency config, versioning management
manual_lib.ng-walkthrough,nz-tour,ui-leaflet,angularjs-slider, andboweritself.Webpackwas added and all dependencies are bundled through it (this is necessary for any React workspace).angular-react-helperwas added in order toangularize()React components.Replacing libraries
ui-leafletwas an AngularJS implementation of Leaflet maps. Sooner or later, we were going to need to make a React component to do this instead. So,LeafletViewnow encapsulates all the logic needed to display maps. For now, it's rendered with React DOM. Eventually, when we stop using Cordova, this will need to be wrapped in native WebViewsreact-native-web-webviewi18next. There is an AngularJS implementation (ng-i18next) and a React implementation (react-i18next) so it's easy to use in both frameworks, but the underlying engine is shared globally.