Unified diary screen #870

tyleryandt18 · 2022-08-05T18:04:20Z

Changes Include:

Design layout of Unified Diary Screen --- CSS color schematics, font styles
Inline map from diary screen
converting trip to tripgj for map in the trip item directive
removing Diary tab from ion-tab in the bottom
Date Line - Breaks: displaying the date in between trips in the infinite scroll

Still working on implementing the date selector.

Issues:

Tripgj percentages: the DiaryHelper.getPercentages does not work in converting trip to tripgj in the trip item directive. The trip does not have an attribute "sections", and looking into the services.js led me down a rabbit hole of trying to implement different functions from the Diary Helper. If one function needed an attribute, I went to the DH to see if I could add that to the trip, and then to do so that function required a different attribute. To add that attribute, the process repeated, so I am shelving the percentages for later.

Pulled out functionality from the InfiniteDiaryListCtrl into the new module, 'infscrolltripitem'. This new scope includes the surveyOpt, itemHt, and showDetail function for the new directive. This relates to issue e-mission#740, Unifying label and diary views.

Created a new module for a diary item directive. Pulled out the necessary functions; created the diary_list_item.html as a template for the directive.

These changes include the stylistic changes to the css layout of the unified diary screen, changes to the multi-label button layout, changes to the java script of the trip item directive, and removing the diary tab from the bottom ion-tab. Issues - - reading in the trip percentages; When I attempted to use the DiaryHelper.getPercentages in converting the trip object to geojson in the trip item directive, the tripgj did not have a sections attribute. I then went into services to see if I could add this, and then I went down a rabbit hole of different functions missing different attributes. I will attempt to fix this soon.

shankari · 2022-08-06T15:28:42Z

@tyleryandt18 I don't see the commit history from the savoir-faire linux codebase.

Please note my comment:

in fact, I would pull from their repo and branch so you get their commit history as well before refactoring

without the commit history, we don't know the reason for each change, and we can't easily make additional changes.

It is very tempting from a coding perspective to check in a giant commit with tons of changes, but it is very problematic from a maintenance perspective.

shankari · 2022-08-06T15:35:21Z

@tyleryandt18 Can you also indicate the testing that you did?
I want to make sure that you have tested all the functionality that you have ported over.
Testing can sometimes take as much time as the original implementation!
Please post screenshots.

In particular, does the draft functionality actually work? My guess is not.

tyleryandt18 · 2022-08-06T16:35:50Z

@shankari I'm not sure on how to merge the Savoir Faire Linux commit history. If I didn't fork the repo from them, then I'm not sure how to merge from them. If you could please outline some steps, then I can go ahead and fetch their commit history.

A lot of the functionality testing is visual. I tested it on both Android and iOS. For example, the in-line map is there, the date breaks are there, the filter tabs still work, and the showDetail() function still works. I will make sure to include lots of in-code testing for the date selector that I implement over the weekend!

This is how I tested the isDraft. I know it's elementary, but I'm not sure how else to test it. It displays false for each trip, which at least leads me to believe that the function is setting trip.isDraft to something. Now, I don't have any drafted trips so I'm not sure how to test the flip side of that.

shankari · 2022-08-06T16:41:25Z

@tyleryandt18

If I didn't fork the repo from them, then I'm not sure how to merge from them.

git pull <repo> <branch> will pull from <repo> and <branch> into the current branch on your laptop

I would pull into a new branch just to ensure that you are not messing up your current changes, and then copy over your changes from this branch into the new branch.

shankari · 2022-08-06T16:42:09Z

This is how I tested the isDraft. I know it's elementary, but I'm not sure how else to test it. It displays false for each trip, which at least leads me to believe that the function is setting trip.isDraft to something. Now, I don't have any drafted trips so I'm not sure how to test the flip side of that.

Are you sure you don't have draft trips? Take a trip that ends at around 30 mins after the hour and then check the diary immediately afterwards. The trip should stay as draft for the next 30 mins at least.

shankari · 2022-08-06T17:07:20Z

A lot of the functionality testing is visual. I tested it on both Android and iOS. For example, the in-line map is there, the date breaks are there, the filter tabs still work, and the showDetail() function still works. I will make sure to include lots of in-code testing for the date selector that I implement over the weekend!

Can you also upload a video showing that the android version works? Even if you scroll by clicking on the map...

tyleryandt18 · 2022-08-06T17:42:18Z

Here is the Android Screen Recording:
Screen Recording 2022-08-06 at 11.39.40 AM.zip

tyleryandt18 · 2022-08-06T18:10:47Z

@shankari I can't seem to figure out how to get the commit history with git pull. My mamobilite repo is in Documents/GitHub/mamobilite, and my phone repo is in Documents/GitHub/e-mission-phone. When I do git pull, it says that the mamabolite Dev branch does not exist, and when I list out the branches that I have access to with git branch -r, it doesn't appear there either. This makes sense to me, but I'm not sure how to then pull their commit history.

shankari · 2022-08-06T18:24:01Z

what is the actual command that you used?

tyleryandt18 · 2022-08-06T18:27:40Z

what is the actual command that you used?

cd Documents/GitHub/e-mission-phone
At this point I was in a new branch. Confirmed by git status --- resulting in Unified-Screen-with-Commit-History
git pull Mamobilite Dev
resulting in fatal: 'mamobilite' does not appear to be a git repository fatal: Could not read from remote repository.

shankari · 2022-08-06T18:30:27Z

You need to use the remote repo

git pull https://github.com... Dev

Not yet added displaying the trips only for that date

tyleryandt18 · 2022-08-06T20:03:12Z

I just returned from the gym, and OpenPATH on my phone records a drafted trip, but not the devapp. This occurs on both the master branch and my Unified Diary Screen branch.

Furthermore, I use git pull https://https://github.com/FabmobQC/mamobilite Dev, and I'm met with fatal: refusing to merge unrelated histories since I guess the their repo is from one fork, and my repo is from another one. So I then use git pull https://github.com/FabmobQC/mamobilite Dev --allow-unrelated-histories`, and therefore create 220 merge conflicts.

shankari · 2022-08-07T01:16:40Z

I just returned from the gym, and OpenPATH on my phone records a drafted trip, but not the devapp. This occurs on both the master branch and my Unified Diary Screen branch.

That's because the data is still on your phone. Wait ~ 5 mins to push the data, or manually "force sync". Then, in the window between the data being pushed to the server and the pipeline running (at 15 mins past the hour), the trip will show up as draft in the devapp as well.

Note that after the pipeline runs, the trip will no longer be draft. So you have ~ 45 min chunks to work on the feature.

If you were running this against a server on your local laptop, you could control when the pipeline runs. Not sure if you want to make that change to your setup for now, though.

shankari · 2022-08-07T01:26:10Z

So I then use git pull https://github.com/FabmobQC/mamobilite Dev --allow-unrelated-histories`, and therefore create 220 merge conflicts.

That's why I suggested that you make the changes in a separate branch.

Did you make your new branch from Unified-Data-Screen or master? I would make a branch off master, merge their changes and then re-apply your changes manually.

You can also ask them to merge everything from master -> a branch on their repo so you could pull from it. But I don't know if they can do this over the weekend.

Or you can resolve the merge conflicts that you have; they are probably fairly simple - your choice!

Note that if you don't resolve the merge conflicts now, then they will have to deal with them when they pull our changes, which is a disincentive to pull our changes and will lead to diverging code branches.

you are not done with the change unless it has the commit history from savoirfairelinux.

I would do this first before making additional changes and leading to even more merge conflicts

shankari · 2022-08-07T01:34:46Z

LMK when you are done fixing the commit history and the draft trips. I'd hoped you would be able to figure why the percentages don't work, but at this point, I might just have to tell you the answer and make some server-side changes.

tyleryandt18 · 2022-08-09T21:23:21Z

Current update, just so that is documented on this GitHub thread. @shankari knows that I am unable to log onto my work laptop, but since OpenPATH is opensource, I can access my changes from my personal laptop. I have created a branch that has pulled the commit history from SavoirFaireLinux. I then merged the conflicts manually, and then pulled the changes from this branch, committed, so that the branch has the commit history and my changes made. I recall that there was an issue with this new branch, but am now unable to recover and investigate that issue since I am locked out of my work laptop. I will now test it on my personal laptop, but I don't know how to convert line endings from dos to unix to run the shell scripts to setup and activate the server. Investigating now.

tyleryandt18 · 2022-08-09T21:39:44Z

https://phoenixnap.com/kb/convert-dos-to-unix#:~:text=Another%20way%20to%20convert%20a,for%20translating%20or%20deleting%20characters.

I have gone through all of these options to try and convert the line endings in order to run a server, but to no avail. I can submit a pr for the branch that has the commit history and changes on it, but I know for a fact that there was an issue when I tried to run it. I can also merge the branches again, but at the current moment, I am unable to test the code. This is why I had switched to a Mac work laptop.

shankari · 2022-08-09T21:44:46Z

I will now test it on my personal laptop, but I don't know how to convert line endings from dos to unix to run the shell scripts to setup and activate the server. Investigating now.

What concretely is the error that you are getting?
What are you using to run the scripts?

tyleryandt18 · 2022-08-09T21:48:22Z

I will now test it on my personal laptop, but I don't know how to convert line endings from dos to unix to run the shell scripts to setup and activate the server. Investigating now.

What concretely is the error that you are getting?

What are you using to run the scripts?

I have ran it on a Cygwin terminal and a Windows command prompt. Quite honestly I'm not sure what the difference between the two are, but I believe that they both execute Linux.
line 2: $'\r': command not found

shankari · 2022-08-09T21:49:56Z

windows command prompt does not execute linux unless you have WSL
which environment do you get that error on (cygwin or windows)?
have you tried gitbash per my previous suggestion?

tyleryandt18 · 2022-08-09T21:58:25Z

I was able to run bash setup/setup_serve.sh and source setup/activate_serve.sh successfully(?) on the cygwin terminal. At least I did not get any line ending errors. But now when I use npm run serve, the error that I am receiving is 'phonegap' is not recognized as an internal or external command, operable program or batch file. I will look into git bash now.

tyleryandt18 · 2022-08-09T22:03:06Z

The same issue appears in the GitBash terminal. 'phonegap' is not recognized as an internal or external command, operable program or batch file.

shankari · 2022-08-09T22:44:46Z

I don't see any output from setup/setup_serve.sh
Are you sure it actually set it up?

you should get something like

$ bash setup/setup_serve.sh
Ensure that we fail on error
Installing the correct version of nvm
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 13527  100 13527    0     0  32912      0 --:--:-- --:--:-- --:--:-- 32832
=> nvm is already installed in /Users/kshankar/.nvm, trying to update using git
=> => Compressing and cleaning up git repository

=> nvm source string already in /Users/kshankar/.bashrc
=> bash_completion source string already in /Users/kshankar/.bashrc
=> Installing Node.js version 14.7.0

Did you get any other errors?

If not, you may need to install node.js 14.7.0 manually since probably nvm doesn't work for windows

After installing node.js manually, you should comment out everything between these lines:

echo "Installing the correct version of nvm"
...
echo "Installing the correct node version"
nvm install $NODE_VERSION

in setup/setup_shared.sh

and try to set up again

shankari

Looks pretty good overall.

I have not recommended any substantive changes, since we don't have time to make them anyway. My comments are primarily around making it easier for others to pick this up later, and focus on:

describing the rationale for the changes,
removing commented out code
justifying the addition of new dependencies

Please make sure to handle the "25 hidden comments" as well.

www/css/main.diary.css

www/js/diary/infinite_scroll_list.js

www/js/diary/infinite_scroll_trip_item.js

www/templates/diary/diary_list_item.html

www/templates/diary/list.html

www/templates/diary/trip_list_item.html

shankari · 2022-08-10T01:27:45Z

@tyleryandt18 After you address all these comments, and you finish the pull from saviorfairelinux + resolving merge conflicts, please re-test with multiple form factors and submit screenshots. See the form factor comments.

shankari · 2022-08-10T02:11:10Z

Glad to see that you have made some changes. However, just to re-clarify the expectation:

@tyleryandt18 After you address all these comments, and you finish the pull from saviorfairelinux + resolving merge conflicts, please re-test with multiple form factors and submit screenshots. See the form factor comments.

#91d4ff -> #80D0FF

tyleryandt18 · 2022-08-10T23:41:48Z

@shankari wrt to the issues that I am having running a server, the package that is missing, according to the debugger console, is emission.main.control.collection. I use bower install emission.main.control.collection, and the terminal states that the package cannot be found. How should I fix this?

tyleryandt18 · 2022-08-10T23:50:16Z

@shankari wrt to the issues that I am having running a server, the package that is missing, according to the debugger console, is emission.main.control.collection. I use bower install emission.main.control.collection, and the terminal states that the package cannot be found. How should I fix this?

I found a similar issue here. e-mission/e-mission-docs#275. But when I run npm run setup-serve, I receive the following errors.

shankari · 2022-08-11T00:02:53Z

$#$#$# windows
Try running the scripts listed there manually - e.g. bash ./bin/setup_autodeploy.js

tyleryandt18 · 2022-08-11T00:06:01Z

$#$#$# windows Try running the scripts listed there manually - e.g. bash ./bin/setup_autodeploy.js

Preaching to the choir. Here is the results.
$ bash ./bin/download_settings_controls.js
./bin/download_settings_controls.js: line 3: syntax error near unexpected token "("
./bin/download_settings_controls.js: line 3: ``var https = require('https');'

shankari · 2022-08-11T00:14:14Z

node ./bin/download_settings_controls.js
bash ./bin/setup_autodeploy.js

one is a javascript file. The other (in spite of the file extension) is a bash script.

tyleryandt18 · 2022-08-11T03:28:56Z

@shankari Android Studio and Windows has not been kind to me. I have a poor quality laptop, so I don't know if that is the issue here or not. I have set looking at my laptop for the past hour. My android emulator is continuously trying to connect to the server, alternating between the "tap to cancel" screen and "extracting" screen. I successfully launched the app earlier, but I switched over to a different branch. I had pulled the from the savoir faire linux branch onto said branch, then pulled my Unified-Diary-Branch onto said branch, and now am trying to test it just to ensure the merging worked fine. But I cannot test it if my computer cannot connect to the server. I don't know what to do, it simply is stuck trying to connect, and there are no console log errors. I have to wake up at 5 tomorrow for my flight, so I am done for tonight.

shankari · 2022-08-11T06:26:05Z

@tyleryandt18 please:

address all the pending changes here (mainly additional comments to the code)
submit the pull + resolved merge conflicts from the Savoir-faire linux repo in a separate PR
ask @sebastianbarry to help you test out the changes. maybe if you ever visit colorado again, you can take him out for coffee 😄

sebastianbarry · 2022-08-19T16:37:11Z

I tested out the changes on an Android device, and made sure the features implemented work including

Infinite scroll
New unified Label/Diary tab
Labeling from the label/diary tab
Going into the trip details and labeling
The profile tab and it's buttons work

A	B	C

A	B

My recommendations

I noticed in the Dashboard tab that one of the texts is still the old teal-color (see screenshot above)
To go into the trip details, you must click the 3 dots. I feel like it would make more sense to bring you into the trip details screen if you click anywhere on the trip tile, not just on the 3 dots
The color for the buttons on the Profile tab seems a bit too light colored to me personally. I feel like we'd benefit from making the red button a little darker red, and the gray buttons a little bit darker gray

tyleryandt18 · 2022-08-19T16:41:36Z

Thank you @sebastianbarry! It was a pleasure working this summer with both of you! Thank you for all your assistance.

shankari · 2022-08-19T17:54:00Z

@sebastianbarry thank you so much for doing this @tyleryandt18 definitely owes you one!

Now, can you also make sure that the changes in the related PR (with the Ma Mobilitie changes) work?
Make sure to do so in a separate directory (it is already in a separate branch) so if there are any issues, you can compare them and pick the correct one.

For example, I seem to remember that Ma Mobilitie had used an action sheet instead of a popup; that might be a change that we want to keep from their side, as our mode string lengths keep increasing ("e-car, with others") is pretty long.

shankari · 2022-08-19T18:36:25Z

@sebastianbarry, @tyleryandt18 has already done most of this in #871
so you should probably just start with that and see if it Just Works.

shankari · 2022-08-19T18:57:24Z

@sebastianbarry since @tyleryandt18's internship has ended, can you make the following changes?

implement your recommendations they seem like they are fairly simple and should not take too long.
restore the diary screen but with the diary item directive we can't actually remove the diary screen since we don't yet have the ability to jump to a particular date or show draft trips or show trip percentages. Once we do that, this change is essentially an improved label screen (correct me if I am wrong).

Let's merge this such, so we don't have lots of pending PRs sitting around, and then address the remaining unification changes in separate PRs.

sebastianbarry · 2022-08-24T04:36:44Z

I made the below changes in this Commit

I noticed in the Dashboard tab that one of the texts is still the old teal-color (see screenshot above)
To go into the trip details, you must click the 3 dots. I feel like it would make more sense to bring you into the trip details screen if you click anywhere on the trip tile, not just on the 3 dots
The color for the buttons on the Profile tab seems a bit too light colored to me personally. I feel like we'd benefit from making the red button a little darker red, and the gray buttons a little bit darker gray

I am not sure how to add this commit to the PR however, because I don't have access to edit Tyler's Unified_Diary_Screen branch

sebastianbarry · 2022-08-24T05:05:44Z

I made the below change in this Commit

restore the diary screen

I DID NOT add the new label screen directive in the old diary screen in the interest of time because it was taking me a while to figure out. Also, if we are planning on eventually getting rid of the old Diary screen and incorporating it into this new improved Label (with the Diary screen built in) screen, then it may be a waste of time trying to figure this out.

With this and my previous Commits, we should be good to ship this as-is for testing. Then...

Next steps

Incorporate the trip guessing percentages (car percentage, bike percentage, walk percentage) in the infinite-scroll-trip-item directive
Incorporate the date-picker functionality from the /root/main/diary into the /root/main/inf_scroll screen
Remove the Diary tab from main.html
Change the Infinite scroll tab's translate name from main-inf-scroll.tab to main-diary, and the icon from icon="ion-checkmark-round" to icon="ion-map"

sebastianbarry · 2022-08-29T17:54:10Z

So that we can close this PR, I created a new updated PR on this same issue, containing the changes I made to the branch:

[New] Unified diary screen (adapted from Tyleryandt18's Unified diary screen) #883

shankari · 2022-09-01T22:06:06Z

Closing this since it is superceded by #883

sebastianbarry · 2022-09-23T18:23:14Z

Here is my PR with the current response to Tyler's code review: #883

There are still a handfull of things to go over; any conversation that doesn't end in Confirmed this change as complete still needs to be addressed.

tyleryandt18 added 3 commits July 7, 2022 17:20

Diary Item Directive

1abf352

Created a new module for a diary item directive. Pulled out the necessary functions; created the diary_list_item.html as a template for the directive.

Added Date Selector Functions and Templates

2d62b7a

Not yet added displaying the trips only for that date

shankari requested changes Aug 9, 2022

View reviewed changes

Requested Changes

67817be

Changing Color Schematic

6a9f4cc

#91d4ff -> #80D0FF

Added Comments

0925801

sebastianbarry mentioned this pull request Aug 22, 2022

Review Unified Label/Diary Screen PRs e-mission/e-mission-docs#779

Closed

sebastianbarry mentioned this pull request Aug 29, 2022

[New] Updated Label screen (adapted from Tyleryandt18's Unified diary screen) #883

Merged

shankari closed this Sep 1, 2022

Unified diary screen #870

Unified diary screen #870

Conversation

tyleryandt18 commented Aug 5, 2022

shankari commented Aug 6, 2022

shankari commented Aug 6, 2022

tyleryandt18 commented Aug 6, 2022

shankari commented Aug 6, 2022 • edited Loading

shankari commented Aug 6, 2022 • edited Loading

shankari commented Aug 6, 2022

tyleryandt18 commented Aug 6, 2022

tyleryandt18 commented Aug 6, 2022

shankari commented Aug 6, 2022

tyleryandt18 commented Aug 6, 2022

shankari commented Aug 6, 2022 • edited Loading

tyleryandt18 commented Aug 6, 2022

shankari commented Aug 7, 2022 • edited Loading

shankari commented Aug 7, 2022 • edited Loading

shankari commented Aug 7, 2022

tyleryandt18 commented Aug 9, 2022

tyleryandt18 commented Aug 9, 2022

shankari commented Aug 9, 2022

tyleryandt18 commented Aug 9, 2022

shankari commented Aug 9, 2022

tyleryandt18 commented Aug 9, 2022

tyleryandt18 commented Aug 9, 2022

shankari commented Aug 9, 2022 • edited Loading

shankari left a comment • edited Loading

Choose a reason for hiding this comment

shankari commented Aug 10, 2022

shankari commented Aug 10, 2022

tyleryandt18 commented Aug 10, 2022

tyleryandt18 commented Aug 10, 2022

shankari commented Aug 11, 2022 • edited Loading

tyleryandt18 commented Aug 11, 2022 • edited Loading

shankari commented Aug 11, 2022

tyleryandt18 commented Aug 11, 2022

shankari commented Aug 11, 2022 • edited Loading

sebastianbarry commented Aug 19, 2022 • edited by shankari Loading

My recommendations

tyleryandt18 commented Aug 19, 2022

shankari commented Aug 19, 2022

shankari commented Aug 19, 2022

shankari commented Aug 19, 2022 • edited Loading

sebastianbarry commented Aug 24, 2022

sebastianbarry commented Aug 24, 2022

Next steps

sebastianbarry commented Aug 29, 2022 • edited Loading

shankari commented Sep 1, 2022

sebastianbarry commented Sep 23, 2022

shankari commented Aug 6, 2022 •

edited

Loading

shankari commented Aug 6, 2022 •

edited

Loading

shankari commented Aug 6, 2022 •

edited

Loading

shankari commented Aug 7, 2022 •

edited

Loading

shankari commented Aug 7, 2022 •

edited

Loading

shankari commented Aug 9, 2022 •

edited

Loading

shankari left a comment •

edited

Loading

shankari commented Aug 11, 2022 •

edited

Loading

tyleryandt18 commented Aug 11, 2022 •

edited

Loading

shankari commented Aug 11, 2022 •

edited

Loading

sebastianbarry commented Aug 19, 2022 •

edited by shankari

Loading

shankari commented Aug 19, 2022 •

edited

Loading

sebastianbarry commented Aug 29, 2022 •

edited

Loading