Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

💄 Minor cleanup of rewrite #923

Closed
shankari opened this issue Jun 23, 2023 · 28 comments
Closed

💄 Minor cleanup of rewrite #923

shankari opened this issue Jun 23, 2023 · 28 comments

Comments

@shankari
Copy link
Contributor

shankari commented Jun 23, 2023

While deploying the code in
e-mission/e-mission-phone#974
e-mission/e-mission-phone#980
e-mission/e-mission-phone#990

There are several minor fit and finish issues. If feasible, we should fix them, test and maybe even push to production before moving on

Missing translation Incorrect formatting Map tiles not loading Incorrect formatting (android only)
simulator_screenshot_03FB26D9-6B0C-418B-BB93-C20E096B8781 simulator_screenshot_26C710B4-3ABC-41F8-9C14-D938B6B40E83 simulator_screenshot_4488BDFD-46DE-40E3-94F1-424ADC96B6E4 Screenshot 2023-06-22 at 5 27 02 PM
@shankari
Copy link
Contributor Author

The map tiles are not loading because of

Some resources are blocked because their origin is not listed in your site's Content Security Policy (CSP). Your site's CSP is allowlist-based, so resources must be listed in the allowlist in order to be accessed.
A site's Content Security Policy is set either as via an HTTP header (recommended), or via a meta HTML tag.
To fix this issue do one of the following:
(Recommended) If you're using an allowlist for 'script-src', consider switching from an allowlist CSP to a strict CSP, because strict CSPs are more robust against XSS . See how to set a strict CSP .
Or carefully check that all of the blocked resources are trustworthy; if they are, include their sources in the CSP of your site. ⚠️Never add a source you don't trust to your site's CSP. If you don't trust the source, consider hosting resources on your own site instead.
Resource Status Directive Source Location
https://tile.openstreetmap.org/15/5271/12709.png blocked img-src index.html#!/root/main/inf_scroll:0
https://tile.openstreetmap.org/15/5272/12709.png blocked img-src index.html#!/root/main/inf_scroll:0
https://tile.openstreetmap.org/15/5271/12708.png blocked img-src index.html#!/root/main/inf_scroll:0
https://tile.openstreetmap.org/15/5272/12708.png blocked img-src index.html#!/root/main/inf_scroll:0

@shankari
Copy link
Contributor Author

@JGreenlee

I am tempted to push this to staging so that we can come up with the full list of issues and fix all of them before going on to the more complicated parts of the rewrite

@JGreenlee
Copy link

@JGreenlee

I am tempted to push this to staging so that we can come up with the full list of issues and fix all of them before going on to the more complicated parts of the rewrite

I am ok with pushing it to staging now so we can track more issues sooner. By now, #990 is fairly old and I'm already pretty far ahead of it. I think many of the issues we will see, I have already fixed in the process of rewriting -- but it would be good to get a complete list.

I expect to have the bulk of the label screen conversion wrapped up tomorrow or this weekend, and maybe that can be in our next staging release.

@shankari shankari changed the title Minor cleanup of rewrite 💄 Minor cleanup of rewrite Jun 23, 2023
@shankari
Copy link
Contributor Author

shankari commented Jun 24, 2023

Onboarding survey on android Mode spacing is all messed up on android
Screenshot 2023-06-23 at 4 47 01 PM Screenshot 2023-06-23 at 5 32 49 PM

@JGreenlee
Copy link

The above issues will be addressed, if not already addressed, in e-mission/e-mission-phone#993 for the next staging release

TODO:

Map tiles not loading - This only affects builds and not the devapp. The fix might be as simple as updating <meta> tags in index.html which sets Content Security Policy. I noticed a whitelist currently includes nominatim.openstreetmap.org, but not tile.openstreetmap.org

Sections / percents indicator - Actually, I still need to implement this indicator altogether on e-mission/e-mission-phone#993. I was waiting for e-mission/e-mission-server#917, which is now merged, so I should now be able to test against the complete implementation.
--I do know what caused this issue (I used the CSS translate, a shorthand for transform: translate(), which is a standard CSS property now but not supported in old versions of Android's Webview. I'll avoid it in case anyone hasn't updated their phone in a couple years)

Try to reproduce later:

Demographics survey formatting - I was unable to reproduce this on the branch of e-mission/e-mission-phone#993. I likely fixed it inadvertently, but we should double check on real devices on the next staging version

Resolved:

Missing 'welcome' translation - just fixed in e-mission/e-mission-phone#993

QRCode formatting - previously fixed in e-mission/e-mission-phone#993

Datepicker formatting - fixed inadvertently in e-mission/e-mission-phone#993 when I migrated it to React.
--The cause was definitely the aforementioned CSS translate shorthand

shankari added a commit to e-mission/e-mission-phone that referenced this issue 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
@JGreenlee
Copy link

JGreenlee commented Jun 26, 2023

Fixed map tiles and added modes / percentages indicator.

(In e-mission/e-mission-phone#993)

All the above issues have been addressed.

@shankari
Copy link
Contributor Author

shankari commented Jun 29, 2023

I'm still seeing the following issues/comments:

Formatting of save token screen on android Not sure this is an issue (Label screen formatting seems to be fine although it is still not 100% clear from the color and icon that this is a bus as opposed to a car)
Screenshot 2023-06-29 at 9 59 19 AM Screenshot 2023-06-29 at 10 00 14 AM

@shankari
Copy link
Contributor Author

I'm going to push this out now and submit my changes to the build when I return

@Abby-Wheelis
Copy link
Member

I just updated my app and the profile page is broken on both test phones (ios and android). All strings are in the format {{'control.edit-demographics'}} and none of the buttons work. The screen is also not scrollable on either phone.

The label and dashboard screens look and behave fine, the profile screen is the only one having those issues. You can sort of see this picture, I don't have email set up on a test phone so I could not get a proper screenshot up

@JGreenlee
Copy link

I can look at this myself, but do you want to try @Abby-Wheelis? It may be good to learn how to debug a physical device

@shankari
Copy link
Contributor Author

shankari commented Jun 29, 2023

I can verify this happens on my phone. It also happens in the emulator with the NREL OpenPATH app.

@JGreenlee @Abby-Wheelis if you can get this fixed by EOD today, I can push out another release with this fix and with the changes in e-mission/e-mission-phone#998

@shankari
Copy link
Contributor Author

Error:

bundle.js:2 Error: [$injector:unpr] Unknown provider: eProvider <- e <- ControlCollectionHelper
http://errors.angularjs.org/1.6.7/$injector/unpr?p0=eProvider%20%3C-%20e%20%3C-%20ControlCollectionHelper
    at bundle.js:2:128028
    at bundle.js:2:151020
    at Object.s [as get] (bundle.js:2:152598)
    at bundle.js:2:151107
    at s (bundle.js:2:152598)
    at l (bundle.js:2:152903)
    at Object.invoke (bundle.js:2:152991)
    at Object.$get (bundle.js:2:151644)
    at Object.invoke (bundle.js:2:153237)
    at bundle.js:2:151127

@shankari
Copy link
Contributor Author

shankari commented Jun 29, 2023

So this may actually be related to the native build when the plugin-specific js files are not copied over.
Note that, by default, the settings files (collect-settings.js and sync-settings.js) are not in the e-mission-phone repo.
They are installed as part of the plugin install.

I hacked index.js to refer to these files

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';

But I bet that the change to optimize the webpack with hardcoded directories (e-mission/e-mission-phone@11fc98c) broke it.
Let's try copying it manually and see if that fixes it.

@JGreenlee
Copy link

@shankari I bet collect-settings.js and sync-settings.js didn't get the babel-plugin-angularjs-annotate treatment

@JGreenlee
Copy link

Yeah they are using the implicit annotations. That would cause it.
And explains why it only affects the Profile tab

@shankari
Copy link
Contributor Author

Confirmed that it is not just the file paths. I ran ./bin/download_settings_controls.js manually and then built, and I'm getting the same error.

@JGreenlee
Copy link

@shankari After copying, did you build webpack again or just cordova?

@shankari
Copy link
Contributor Author

webpack too

$ history
./bin/download_settings_controls.js
npm run build
npx cordova emulate android

@JGreenlee
Copy link

@shankari And when you did that, you removed the hack from index.js right?

@JGreenlee
Copy link

I am fairly certain that this error results from collect-settings.js and sync-settings.js not getting 'fixed' by the babel plugin

But if they were copied over, as long as they were in the www/ directory they should have been annotated the second time

@shankari
Copy link
Contributor Author

shankari commented Jun 29, 2023

Fixed:
Screenshot 2023-06-29 at 2 09 11 PM

I copied the files over, but didn't revert the index.js change to include the non-platform locations.
After changing that as well, everything worked.

@JGreenlee
Copy link

Whew!

So, sounds like for the build configuration, we just need download_settings_controls.js to be run before webpack is run.
Should I add that at the front of the npm build script?

@shankari
Copy link
Contributor Author

I can add it, I already have a change for the android release build pending...

@JGreenlee
Copy link

JGreenlee commented Jun 29, 2023

TODO

Here's what I've noticed:

  • the most recent trip is always left off. You have to "Show More Travel" to see it or any draft trips
  • "Show Older Travel" and "Show More Travel" are not internationalized
  • when you load more trips, their address names don't fill in
  • activities don't show up until after a refresh
  • some 'mode' icons (I noticed 'car' and 'bike') are showing up cropped / too big for their bounding box

  • activities cannot be edited or deleted

  • on the details page map, the zoom buttons don't do anything
  • if there is a gap in the timeline of 1 week or more, you cannot navigate past it; the 'Show Older Travel' button will keep fetching the empty week over and over

  • if a request comes back with no trips, there's no indication and the spinner never goes away
  • multiple missing translations. In French and Italian, because these are not maintained, there are so many missing that it actually causes the app to hang.
    -- Ideally if a translation is not available, we should fall back to English, rather than showing the raw translation key
  • Map sometimes overlaps time badges (noticed on iPhone 8)

  • In the time-use survey, the check boxes just highlight the outline of the box, don't actually add a check mark so I couldn't tell that I had selected it

  • Don't have walk and car be shades of red since they look similar, could be hard for people with red/green blindness

  • No boundary saying when your first trip was; I could go back to 2020 in the calendar and then it just hung and the dates were showing up as 2023/06/29 to 2023/06/29

@shankari
Copy link
Contributor Author

the most recent trip is always left off. You have to "Show More Travel" to see it or any draft trips

Following up on this, the last trip is also displayed incorrectly (on my phone). My last trip was from Palo Alto to home, but the map in the list view shows a walk around Palo Alto. The detail screen shows the correct trip though (see screenshots).
Also, the map in the list view seems to be zoomable. I'm not sure if that is a good or a bad idea. Is it confusing/does it happen that we try to scroll but the map zooms instead.
And also, the place names are not showing up.

List view (wrong) Detail view (right) Zoomed list view (???)
Screenshot_20230629-152356 Screenshot_20230629-152424 Screenshot_20230629-152503

@shankari
Copy link
Contributor Author

shankari commented Jun 30, 2023

Comments from @mRaffill

  • Don't have walk and car be shades of red since they look similar, could be hard for people with red/green blindness
  • Really like how the modes are showing up; that is most helpful part of the diary; not too big and not too hidden either - that's great
  • looks a lot more professional now
  • Don't like how "Add trip details" and "Add Activity" are are slightly offset - looks like you tried to make them aligned but did something wrong with the spacing and it didn't work
  • In the time-use survey, the check boxes just highlight the outline of the box, don't actually add a check mark so I couldn't tell that I had selected it
  • "show more travel" or "show older travel" is good; much better than the weird download button before (@shankari's UI design skills... shakes head and laughs)
  • I really like that you can see the drafts without going into a separate diary
  • "This looks so good actually" Such a big step up from the previous UI
  • No boundary saying when your first trip was; I could go back to 2020 in the calendar and then it just hung and the dates were showing up as 2023/06/29 to 2023/06/29
  • If you do later (e.g. December) just shows draft trips and "show older travel". Dates are still showing as 2023/06/29 to 2023/06/29
  • once I set a valid date, it worked again and it makes sense
  • using "clear" on the calendar dropdown causes the app to hang again

@JGreenlee
Copy link

  • No boundary saying when your first trip was; I could go back to 2020 in the calendar and then it just hung and the dates were showing up as 2023/06/29 to 2023/06/29
  • If you do later (e.g. December) just shows draft trips and "show older travel". Dates are still showing as 2023/06/29 to 2023/06/29

Yeah... that's another reason we will need to use react-native-paper-dates sooner or later.

https://caniuse.com/?search=input%20date

As much as I love how the native iOS datepicker looks, it doesn't offer any way to constrain inputs. If I put min and max attributes, it would work great on Android (days outside the range would be disabled), but iOS would just ignore it.

@shankari shankari moved this to Current two week sprint in OpenPATH Tasks Overview Jul 6, 2023
@shankari
Copy link
Contributor Author

Closing this since e-mission/e-mission-phone#1000 is merged
Next round of cleanup will be tracked in #932

@github-project-automation github-project-automation bot moved this from Current two week sprint to Sprint tasks completed in OpenPATH Tasks Overview Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants