🏷️ Migration of Label Screen Components #993
Conversation
In the label screen, the timestamps for each trip/place/untracked are shown at the top and bottom of the card. Since this is used in many places, it has been refactored into a reusable React component. It uses the Badge component from react-native-paper, and applies the appropriate colors from the theme.
This will allow the use of the `useTranslation()` hook in our React components. This is how we will accomplish internationalization in the new React components we are writing. https://react.i18next.com/latest/usetranslation-hook
MultiLabelButtonGroup.jsx will be the React component that handles the multilabel buttons. This is a React rewrite of multi-label-ui. The popup menu is a work in progress; it uses a `Menu` from React Native Paper, but it does not render correctly because of the existing DOM structure that is from AngularJS. We need a way for this to appear on top of all other content.
|
I have now merged #990 |
I finally got icons working with React Native Paper components by following the guide from here: https://callstack.github.io/react-native-paper/docs/guides/react-native-web/ The webpack configuration was more complicated than it needed to be. There was unnecessary config left over from when I tried to get the icons working before, and this can be removed. As a pleasant side effect, I think bundling is now happening much faster.
At this point, the multilabel buttons are now behaving almost exactly the same as they used to - except rendered in React instead of AngularJS. The popups are going to stay as ionic for now. For that to work, we use the $ionicPopover service from within React. This works fine for now. Later, I'd prefer to make these into Menus or Dialogs. But I can't do this until we have some of the upper-level parts of the UI rewritten in React.
The functionality from the multilabel directive has been replaced by the MultilabelButtonGroup component, and the multilabel directive is no longer being used. Inside `multi-label-ui.js`, the MultiLabelService is still being used, so that file has to stick around for a while longer.
This is not being used. Although this would be the ideal, I abandoned this idea because the Menus will not display correctly until we have migrated upper-level UI components.
The 'secondary' colors are being used for 'inferred' labels. For text readability, I'm using more of an orange than a yellow. I'm also tweaking the primary colors in accordance so that we overall have a slightly more pleasant palette.
This popup is now triggered from MultilabelButtonGroup and this function is never used anywhere.
|
The multilabel buttons, and the logic necessary for their behavior, have been consolidated into I did not create a separate component for the checkmark, since it's just a one-liner
As you can see, I opted to make the checkmark button show with an outline (this way it actually looks like something you can press); and I also shifted the color palette with the goal of having a better contrast ratio for readability |
|
Much of the hardest work is out of the way. I think that creating components for the diary cards will not be difficult, as the only non-React elements left are just text. Similarly with the details page. Migrating the label screen may not take as long as I had anticipated. But first, I want to stop and write unit tests for what's here. |
Adding TypeScript to the project required a bit of webpack reconfiguration. It also made bundling extremely slow (~60s). So, I sifted through the webpack.config.js and optimized it as much as possible. Now, bundling is taking about ~12s.
This allows VSCode's IntelliSense to not have so many files to watch
VSCode wanted me to explicitly add 'children' before on these elements, but I think it was due to a bug. Once I got TypeScript fully configured, and the TypeScript extensions were reloaded, this was deemed unnecessary, and it gave me a warning to remove 'children' from the definition.
This allows us to 'import React from "react"', instead of having to 'import * as React from "react"'. In this project we want to be consistent about using ES import syntax, preferably treating everything as either a default or named export. esModuleInterop allows us to do this even if the libraries differ.
|
I have not been able to set up component testing. I burned the better part of yesterday trying to find the right configuration. I exhausted all the help I can find on StackOverflow + GitHub, and I'm not sure that me continuing to wrangle with it is going to be beneficial anymore. It seems darn near impossible to get Jest to work in our project at this time - there are too many complications: TypeScript, Ionic/Angular legacy packages, the fact that we are using React Native Web in a nonstandard way.. It's not going to be easy to implement quality testing until we are free of Cordova and are working in an Expo ecosystem. That said, I think we can still test simple, individual functions easily; i.e we can make sure functions give the correct output when given a fake input. But I'm not sure how valuable that kind of testing is going to be in this codebase, because at that point we have to mock almost everything. I know that testing is really important because of the large-scale nature of the project, but unless someone else can get component testing working, we may have to settle for the more primitive kind of unit tests. |
Since we are going to keep using the ionic popovers until the rest of the label screen is rewritten, we might as well make them look nicer.
This has been replaced by the new checkmark icon in MultilabelButtonGroup.
This component replaces enketo-add-note-button. Most of the logic is kept the same.
The new AddNoteButton.tsx has a plus sign icon next to the text. This style tweak is necessary for buttons with icons to show correctly; otherwise the button label overlaps the icon.
MultilabelButtonGroup should show if elementTag is 'multilabel', and the trip button should show if elementTag is 'enketo-trip-button'
This directive has been replaced by AddNoteButton.jsx. This file also contains EnketoNotesButtonService, which is still being used, so I cannot remove the entire file as of yet.
This new component replaces enketo-trip-button. It has been made more generic; verbage has been changed to 'timelineEntry' in a few places that it still said 'trip'. The enketoTripButton directive has been removed. As was the case with the other buttons, EnketoTripButtonService is still in use and the enketo-trip-button.js file must be kept for now.
In the Content Security Policy, img-src was set to accept 'self' and 'ionic: data: https://*.tile.openstreetmap.org'. I believe this was necessary for the purpose of using ui-leaflet. Now, we have LeafletView, which is independent of Ionic and AngularJS. It retrieves tiles directly from https://tile.openstreetmap.org. Updating the policy as such resolves the issue.
The `detachPreviousScreen` option is true by default and means that when you navigate to a new screen, the old one is detached so it can be removed from memory. In the case of the label details screen, we are going to go right back to the main label screen from the details page. So we shouldn't detach it. This also fixes the issue of the scroll position resetting when going back from the details page.
There was a problem hiding this comment.
This includes everything EXCEPT LabelTab and TimelineScrollList. Those are large and complex, so I will defer them until tomorrow morning. Note that I have also not reviewed the CSS/SCSS in any level of detail since the proof of their correctness is in the visual representation.
| <View style={{width: 'calc(100% - 20px)'}}> | ||
| {inputKeys.map((key, i) => { | ||
| const input = trip.inputDetails[key]; | ||
| const inputIsConfirmed = trip.userInput[input.name]; |
There was a problem hiding this comment.
do we still have the distinction between trip.userInput and trip.user_input? I have frequently felt that we should rename one or the other just to avoid confusion among future maintainers.
There was a problem hiding this comment.
I didn't rename them. I've also thought it would be a good idea; for additions as well.
Currently, the server gives us user_input and additions, and the phone organizes these into userInput and additionsList
Maybe the phone can do userInputList and additionsList -- or a better descriptive name if we can think of one
There was a problem hiding this comment.
aggUserInput and aggAdditions ?
I'm thinking it's basically an aggregate of processed input(s) from server and unprocessed input(s) on phone
There were changes upstream while I was working on the PR this is a part of. When I pulled them back in and resolved merge conflicts, I overlooked some important changes that had happened with cordova plugins and versioning https://github.com/e-mission/e-mission-phone/pull/993/files#r1244031155 https://github.com/e-mission/e-mission-phone/pull/993/files#r1244628495 https://github.com/e-mission/e-mission-phone/pull/993/files#r1245346066
|
I'm merging, building, and pushing to staging. |
this is needed from changes in e-mission#993
as a feature of e-mission#993 the calls to angularize need the name parameter, up to date and running smoothly!
|
@JGreenlee while building the native app, I have the following error and the app hangs at the splash screen |
|
Upgrading to ui-router to 0.3.1 did not fix the issue |
|
Does it hang on splash immediately on launch, or after onboarding? |
|
on splash immediately after launch EDIT: Stuck on this screen 
You should be able to reproduce if you remove and reinstall the devapp |
|
The instance of When I added Webpack, I did try using other versions of |
|
I'll try building later |
|
For the record, I also tried from https://stackoverflow.com/a/43777868/4040267
I don't think you need to build, just uninstall + reinstall the devapp |
|
For the record, this also happens on iOS (as would be expected) |
|
@shankari Using Next, I tried reproducing the issue on a build. Then Result: 
|
|
Could the issue somehow be specific to the 'NREL OpenPATH' deployment? I only built the 'emission' deployment (because that's all I know how to do) |
|
wait, so you are still getting the error, but the join page still launches?! |
|
I think we will also need to change |
|
FYI 
Which minifies JS and fixes the Angular annotations with the babel plugin I haven't tested dev Webpack + Cordova build |
* switched collect and sync lists over to react * inserted a placeholding div control-list-item This is not yet complete, but is an idea of where the schedule table might eventually be * patch from Jack 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 * added keys to rows 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? * removal of non-relevant code took out the code that was a part of a different effort to prevent confusion, also removed commented out code form replacing elements * replacing hard coded strings 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 * start at storing data in better place 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 * spelling * starting to work with other rows of profile this is where I'm struggling with the parameterization of i18n keys * setting up issue in using key as parameter Added in code so the broken setting row is enabled, note the console.log statement demoing the different ways the info comes through * fixing strings 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]> * text working on sample list item 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 * carry data storage change through switching from plugin storage to scheduler storage Co-authored-by: Jack Greenlee <[email protected]> * polishing up - removing print statements, altering log 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 * update comments, eliminate changes not a part of this PR the setting-row process has been separated to More profile migrations #994 * action parameters for setting rows - some work 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 * comments and spacing 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 * more row migrations most rows now migrated, still need to handle some with slightly unique functionality, styling with icons also needed! * rough draft - two rows unconverted all rows ugly but functional but need guidance on reminder time and demographics button * fixing strings - i18n * enabling icons - copy Jack's related commit 6578d8a * got icons in still missing some visual cues (toggle switches, expansion flippers) * tidy code - still ways to go * combine logout and copy (eliminate copy) * convert to use of IconButtons now the button is what has action, not the entire row - there's a bug requiring an empty onPress in the list Item for the one in IconButton to work see issue callstack/react-native-paper#3898 and hopefully soon to be fix callstack/react-native-paper#3908 * introduce toggle switches the two items that had switches are now interactive again, there is work to be done with getting the switches to have the proper starting value toggle switch: https://callstack.github.io/react-native-paper/docs/components/Switch/ * experiment with list accordian converted the user data section to a list accordian - works great! Unfortunately relies right now on a lot of hard-coding * use of .map for creating the collapsing list! first-run at creating a drop-down part of the menu with map and passing in a data structure * both drop-downs converted to ExpandMenu components fully converted the userData and devZone to the react element, creates an accordian element that has SettingRow and ControlDataTable elements * profile settings component draft * profile settings in general settings * first set settings now in ProfileSettingsComponent creating an over-arching component for the ProfileSettings -- still passing in functions, some things have bugs/functional differences noted in comments * different method of passing info to Profile Settings * introduce Demographics Setting Row * rough sketch everything in ProfileSettings working towards full migration and pulling everything into a single react Component. This is not working perfectly, there are a lot of things to handle that are currently commented out * more of migration to ProfileSettings component removing some commented out code in ExpandMenu.jsx, props is now the listed parameter, this allows to get the default parameter props.children to populate the inner components. the section Title is accessed with props.sectionTitle * adding a value in the scope for the accuracy setting working on the switches, and trying to get their displayed value to match the value of the think they control * adding additional argument to angularize call this is needed from changes in #993 * cleaned up irrelivant codes and comments * Revert "fixing merge conflicts" This reverts commit ffd2bc2, reversing changes made to a3097ac. --------- Co-authored-by: Jack Greenlee <[email protected]>
This PR begins the work of rewriting the label screen components as React components.
Specifically,
TimestampBadge.jsxandMultilabelButtonGroup.jsxare new. (The popup menus thatMultilabelButtonGroupcreates need to be reworked.)(once
framework_migration_prepis merged intomaster_for_platform, I will change the target of this PR tomaster_for_platform)