e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

Unifying label and diary views #740

Closed shankari closed 1 year ago

shankari commented 2 years ago

In angular1, the UI is in .html and the javascript controlling the UI is in .js in a type of class called a controller

The functionality in both of them is almost the same. Our goal is to modularize and unify the two so that we have one codebase and the users have one place to go do label trips.

shankari commented 2 years ago

When I port the tutorial from the diary view into the unified view, perhaps I could add a color schematic into the tutorial. Just a thought.

That sounds OK - let's try that and see if we need anything else. I think that there is actually a tutorial in the label screen as well, I just disabled it because the intern who built it tried to make it really fancy and it didn't fully work by the time he left. https://github.com/e-mission/e-mission-phone/blob/master/www/js/diary/infinite_scroll_list.js#L434

Note that one of the problems is going to be that the walkthrough is useful when you have trips to show what all the components are, BUT when users install the app they have no trips so you can't show anything. You may want to think through an alternate high-level walkthrough as part of the onboarding that uses screenshots from our data/fake data first.

StackOverflow pointed me in the direction of this library, where a user stated that they used it in their Angular1 project https://github.com/mattlewis92/angular-bootstrap-calendar.

This looks like a good option. However, I don't see anybody complaining about the existing calendar in the feedback so I would tag this as lower priority.

shankari commented 2 years ago

@tyleryandt18 let's start making progress on the hard-to-code parts that we know we will need to implement while waiting to hear back from interns, friends and family, focus group...

map created on the fly using subsample

We don't want to make the mistakes of the original diary and pull all the trajectory information at the same time and slow down the loading

pre-created map

In the pre-created map option, we would also retrieve the maps asynchronously and "fill in" the map information but filling it in would require less code on the phone because the browser can deal with asynchronous image loads

Map components

high level pro v con

@tyleryandt18 will start implementing the first approach and see how bad it looks

shankari commented 2 years ago

@tyleryandt18 I just realized that we have to implement the "map created on the fly using subsample" approach anyway in order to support trips that have not yet been pushed to the server (green background aka draft trips).

Ask @sebastianbarry for examples if that is not clear, or look at the difference between diary and label screens right after (~ 5 mins after) you come back from a trip.

shankari commented 2 years ago

@tyleryandt18 Note that, as part of the unification, we also want to make sure that the label screen displays draft trips. That will require a fair amount of javascript refactoring, so let's focus on getting the maps inline for processed trips first and then start on the draft trip display.

shankari commented 2 years ago

@lgharib @PatGendre @asiripanich @jf87 since we are changing the visual representation of anyway while unifying the diary and label screens, we thought we might as well improve it. We were also inspired by the Ma Mobilite redesign which looks a lot cooler than the current UI.

@tyleryandt18 has created these mockups, please vote for which you like best!

Mockup 1 Mockup 2 Mockup 3
Screen Shot 2022-07-18 at 1 33 36 PM Screen Shot 2022-07-18 at 1 39 04 PM Screen Shot 2022-07-18 at 1 44 20 PM
asiripanich commented 2 years ago
Screen Shot 2022-07-14 at 4 16 47 PM

Why is this not an option?

shankari commented 2 years ago

Why is this not an option?

That is pretty close to Mockup #2, right? The main difference is the base color theme which is "NREL blue" 😄 for obvious reasons

asiripanich commented 2 years ago

Fair enough. However, I like that one better because of the position of the start and end times.

shankari commented 2 years ago

Noted. @tyleryandt18 made the mockups using Figma so they are a bit rough. @tyleryandt18 please note the strong vote for just switching to the Ma Mamobilite UI 😄

PatGendre commented 2 years ago

@shankari I agree with @asiripanich , the position of the start/times is clearer on the mockup2 variant :-) Also, it shows a date selector : I think in this issue you want to solve in a generic fashion both the current diary and label (infinite list) views so maybe this is why you don't show explicitly the date in your mockps, however in both cases the date should appear clearly somewhere nearby the trip, in my opinion.

tyleryandt18 commented 2 years ago

@PatGendre @asiripanich @shankari Hello all, I'm working on the unification of NREL's diary and label screen. These mockups reflect a short period of time where I was considering removing the date selector feature altogether, but I can assure you that we will include it, after some thought it seems necessary. I was considering have a filter tab in which you could jump to a specific date from a calendar popup. The final design I'm envisioning will look something along the lines of:

Screen Shot 2022-07-20 at 10 53 21 AM

And yes, as Shankari stated my Figma skills are still pretty elementary haha, that's why you can see some shadows here and there and the buttons aren't lined up perfectly.

Calendar Popup

So my thought process is that when you select the calendar filter, this popup will open and allow you to choose a specific date.

shankari commented 2 years ago

after some thought it seems necessary. I was considering have a filter tab in which you could jump to a specific date from a calendar popup. The final design I'm envisioning will look something along the lines of:

We might also consider having separators in the scroll - e.g.

Jul 10 trip
Jul 10 trip
Jul 10 trip
---- July 11 ----
Jul 11 trip
Jul 11 trip
Jul 11 trip
shankari commented 2 years ago
console.log("Before reading from timeline, $scope.trip = "+$scope.trip);
console.log("using trip id "+stateParams.tripId+" to read the trip");
$scope.trip = ...
console.log("after reading from timeline, $scope.trip = "+$scope.trip);
shankari commented 2 years ago

Issue #1: why is $scope.trip set to undefined in your code? It is defined in the multilabel-UI which logs in the same way.

$scope.trip should be defined in your controller since it is passed in

Next, look up what $stateParams does and how it is used. HINT: as you can guess from the name, it has something to do with the parameters for a state. Look at how we go to the detail screen. What function do we use, and how might it be related to $stateParams?

shankari commented 2 years ago

to debug, check how the multi-label-ui works maybe try to pass in some other parameters and see if they work basically figure out why $scope.trip doesn't get passed through

tyleryandt18 commented 2 years ago

@shankari The investigation into why $scope.trip won't get passed through into the infinite_scroll_trip directive:

And since our trip is mysteriously not being received, when we call that function, it sets the trip to be undefined because the trip object that is being passed in does not have a tripId associated with it. So undefined tripId in the function returns undefined.

tyleryandt18 commented 2 years ago

Oh and when we are retrieving the trip, or attempting to, with the $stateParams tripId, there is nothing there. I'm not sure I understand why this would ever work, even if our trip was being passed in correctly. Is it because $stateParams has access to the attributes of the trip?

shankari commented 2 years ago

Next, look up what $stateParams does and how it is used. HINT: as you can guess from the name, it has something to do with the parameters for a state. Look at how we go to the detail screen. What function do we use to go to the detail screen, and how might it be related to $stateParams?

tyleryandt18 commented 2 years ago

While I'm doing that, could you touch briefly on if the code is using the detail.js file? I was looking into that file, and it goes about formatting its tripgj object ever so slightly, so I was going to see if pulling that code would work.

shankari commented 2 years ago

are you importing the module from detail.js? If not, we don't use anything from it.

tyleryandt18 commented 2 years ago

I'm not importing the module, should I be?

I was just noticing that there is a different method of retrieving the tripgj in that js file. And I was going to try out this code. But, I guess if we aren't using detail.js anymore then the code might be outdated.

Screen Shot 2022-07-22 at 10 12 45 AM

This screenshot is from detail.js.

shankari commented 2 years ago

@tyleryandt18 you are going down the wrong path. Please answer my questions in https://github.com/e-mission/e-mission-docs/issues/740#issuecomment-1192713073 that we also discussed at our 1:1 yesterday

tyleryandt18 commented 2 years ago

Next, look up what $stateParams does and how it is used. HINT: as you can guess from the name, it has something to do with the parameters for a state. Look at how we go to the detail screen. What function do we use to go to the detail screen, and how might it be related to $stateParams?

$stateParams is a service that captures parameters and we can use it to display information according to the state. So when we go to a different state, the parameters would change. For example, the function that we use to go to the detail screen is a $state.go, and then it takes us to the different state and hence the different page. Within that function, we have a tripId: trip.id, so does that establish the tripId as a $stateParameter for that detail screen state? @shankari

shankari commented 2 years ago

@tyleryandt18 yes. so what is the equivalent for the directive? do we need to use the $stateParams?

tyleryandt18 commented 2 years ago

@shankari I'm not sure what you mean by the "equivalent for the directive". But no, we would not need to use $stateParams.tripId for that function. We could just do $scope.trip.id to go and retrieve the confirmed trip. $scope.trip = Timeline.getConfirmedTrip($stateParams.tripId); --> $scope.trip = Timeline.getConfirmedTrip($scope.trip.id)

shankari commented 2 years ago

@tyleryandt18 Exactly, and I would go further: why do you need $scope.trip = Timeline.getConfirmedTrip($scope.trip.id) when you already have $scope.trip?

tyleryandt18 commented 2 years ago

Why do you need $scope.trip = Timeline.getConfirmedTrip($scope.trip.id) when you already have $scope.trip?

Well now that is a great point. We wouldn't need it

shankari commented 2 years ago

Right. If you already have the trip, you just need to convert it to geojson to display it. Directly. Without lookup. So you just need to figure out why the $scope.trip is undefined in the controller and you are done.

I don't know why $scope.trip is not defined in the controller. It seems like it should be given that it is a parameter to the directive. You need to poke at it with different options - you have already tried passing in a string - what if you pass in a different object.... The answer is bound to be something very stupid, probably a typo or an incorrect format issue.

tyleryandt18 commented 2 years ago

@shankari I just tried passing in a different object, and it was able to read it in fine. So it has specifically to do with the trip object, because when I set trip=a string, it was also able to read it in.

shankari commented 2 years ago

I know that showDetail worked for you earlier https://github.com/e-mission/e-mission-phone/pull/853/files#diff-338ab59c91e4724bfe144369b8f692359a4faa73acb869a0f6980ec6985c0cfdR32

if you try to console.log in there, is $scope.trip set?

tyleryandt18 commented 2 years ago

It is only set after retrieving the trip with the tripid from $stateParams, but that makes sense

Screen Shot 2022-07-22 at 11 54 30 AM

.

shankari commented 2 years ago

@tyleryandt18 we are not worried about the detail screen. we know it works. But when the user clicks on the ..., it calls the showDetail function in the directive. Inside that function, is $scope.trip defined?

tyleryandt18 commented 2 years ago

@shankari Right, sorry about that. So here now I added a console.log within the function, so when it's called it'll log. Logging the trip from there produces this result, so it is defined.

Screen Shot 2022-07-22 at 12 01 23 PM

.showDetail()...{ console.log("Testing if showDetail has the trip defined ", $scope.trip)

tyleryandt18 commented 2 years ago

Also, to further prove that point, trip must be defined in order to see the map on the detail screen, which still works despite the map on the inf_scroll screen working. What I mean by this is that we pass the trip id into the $stateParams...

tripId: trip.id Within the showDetail function. Therefore, it must be defined if goes to the detail screen and displays the correct state. This is so weird.

shankari commented 2 years ago

well, arguably, tripId: trip.id uses trip not $scope.trip. But now I know what is going on and it is the async execution that I was talking about earlier. Async operation is key to understanding javascript operation.

To understand what is going on, look at the ordering of the console logs.

After you do all that, read up about $apply and $watch, which are techniques that angular uses to handle async execution. HINT: $watch will be key to the fix.

tyleryandt18 commented 2 years ago

well, arguably, tripId: trip.id uses trip not $scope.trip.

Could you explain this just slightly? I'm not sure what the difference of this would be, because that showDetail function is a $scope function, is it not? So then to access the trip it would be the trip in the scope, and therefore the same as $scope.trip?

shankari commented 2 years ago

because that showDetail function is a $scope function, is it not? So then to access the trip it would be the trip in the scope, and therefore the same as $scope.trip?

showDetail is a $scope function, but you also pass in trip as an argument to it. Inside the function, you currently use trip, which is the passed in argument.

To improve your directive implementation, you could remove that argument and change to tripId: $scope.trip.id

tyleryandt18 commented 2 years ago

@shankari

When is the controller created?

So in the console logs, the infinite scroll controller is created before the trip item controller, the one for my directive. The Trip Item Ctrl is created fairly early on in the process.

Screen Shot 2022-07-22 at 12 32 28 PM

Where do the trips come from and when are they populated?

Well the trips come from the server. https://github.com/e-mission/e-mission-phone/blob/c0395cf3bf981e45257debad82c9c56e87dc4361/www/js/diary/infinite_scroll_list.js#L87 They are populated in the lines following that code, and therefore after the the controllers are called, correct? In the console, the trip item ctrl is called before the list of trips begins to be populated.

When is console.log in showDetails() invoked?

The console.log in showDetails() is only invoked when I click on a button on the screen, so therefore after the controllers are called and the trip list is populated.

shankari commented 2 years ago

great! so what are some approaches for fixing it? My high level thoughts are:

You could also look into event broadcast/listen although IMHO that is not as attractive as the others

Please try out the first two options and list the pros and cons

tyleryandt18 commented 2 years ago

ng-repeat vs collection-repeat

I thought that we had to use collection-repeat in order for it to function on Android Devices?` <!-- This needs to be collection-repeat instead of ng-repeat on android. Otherwise, you can only scroll using the sidebar, scrolling on the map side does not work. I have no clue why this happens, but it is not worth it to understand this deeply. It is not purely a leaflet issue, but it also clearly something to do with leaflet because scrolling on the details part does work.

    If you want to change this from collection-repeat to ng-repeat, you
    MUST test on android first to make sure that the scrolling works.
    -->` 

At least this is if you're talking about the pros and cons of switching it in the inf_scroll list html file.

shankari commented 2 years ago

look at the blame for that line - I wrote it in 2015. So I would try and test it out on android and see if that still holds Maybe first try on iOS to see if it even works there and then try on android

EDIT: also (I know the answer) but why does it work on ng-repeat? What does the async timeline look like in that case?

tyleryandt18 commented 2 years ago

@shankari I went ahead and switched the tripId: trip.id to tripId: $scope.trip.id, and the trip object is still defined when I log it to the console. Is there any significance to that?

tyleryandt18 commented 2 years ago

@shankari I went ahead and switched the tripId: trip.id to tripId: $scope.trip.id, and the trip object is still defined when I log it to the console. Is there any significance to that?

Does this mean, in theory, if I just created a $scope function that changed the trip object to a tripgj, that the trip object would be read in correctly?

I'm going to get to the pros-cons list, I was just writing my write up so I'm just getting back on to this investigation.

shankari commented 2 years ago

I went ahead and switched the tripId: trip.id to tripId: $scope.trip.id, and the trip object is still defined when I log it to the console. Is there any significance to that? Does this mean, in theory, if I just created a $scope function that changed the trip object to a tripgj, that the trip object would be read in correctly?

What do you think? I can tell you the answers, but the questions indicate that you haven't fully internalized what the various variables are and how they are set. Try it out, figure out the answers and then figure out the "why" behind the answers. You will understand better once you actually see it in action.

tyleryandt18 commented 2 years ago

@shankari Pros and Cons List ng-repeat vs collections repeat

shankari commented 2 years ago

@tyleryandt18 why don't you go ahead with fixing the easy fix for the downside then, and make the changes to have the directive look like the preferred mockup at the same time? Add screenshots once you are done.

Edit: The start and end time should really be inside the directive anyway. If it doesn't work on android, we can move on to $watch

shankari commented 2 years ago

@tyleryandt18 has now finished the UI directive changes and will add them to the PR pretty soon along with TODOs.

In terms of reading from the server, we currently read data in

Timeline.readAllConfirmedTrips(currEnd, ONE_WEEK).then((ctList) => {

and we append it to the current list

        $scope.data.allTrips = ctList.concat($scope.data.allTrips);

So the way that we load the data is:

If we wanted to select an arbitrary date (say from a year ago) how should it work?

shankari commented 2 years ago

High level solutions for the issues:

tyleryandt18 commented 2 years ago
  • make sure that if they select a date that they have already selected, we don't read it twice
  • what they go to a year ago and then try to scroll down? Right now we start at the end, and we append to the current list, so there is always data on the phone when we scroll down, until we get to the end. But if we jump to a year ago, then the data from the time between a year ago and now will not be retrieved to the phone. So we won't have it when the user scrolls down.

@shankari I feel as if these two issues/solutions are conflicting. When a user jumps to a date from a year ago, are we allowing them to scroll up and down? I don't believe so, so therefore when they select a date, it just displays that date's trips, correct? So then why are we not allowing them to click on a date that they have already selected? if they use the date selector to go from a year ago, then go to a date like yesterday, why could they not use the date selector to go back to that date in the past?

tyleryandt18 commented 2 years ago

@shankari Furthermore, I don't know if I will be able to complete the date selector functionality. I really don't understand it, and I feel as if there might be a solution to the UI even better than the date selector that Sebastian could implement. If you don't believe that the trip draft function is working, then maybe I should focus on making sure that works along with the trip percentages, because I am also stumped on that implementation, but am confident I can work it out.