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

map fixed, bug fixed #29

Merged
merged 4 commits into from
Apr 11, 2016
Merged

map fixed, bug fixed #29

merged 4 commits into from
Apr 11, 2016

Conversation

krisma
Copy link

@krisma krisma commented Apr 3, 2016

The map now looks smoother, and the start stop icons have been changed to avoid blocking the map content.
A bug is fixed. When users have both running and walking mode, the percentage function, which should return a set of mode, turned out to have duplicate items since walking and running map to the save icon name. It's fixed now so running is filtered and changed to walking before mapping to icon names.
screen shot 2016-04-02 at 10 14 16 pm
screen shot 2016-04-02 at 10 14 33 pm

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@shankari
Copy link
Contributor

shankari commented Apr 3, 2016

This is much better, but it doesn't contain all the little details from the mockups.
For example, in the mockups, the list view has a line under the icon showing the color for that mode and the length represents the % of time in that mode.

And in the detail view, the time of the trip is also shown at the top, and there's a list of sections shown below the map with more details on their start and end times.

Do you think that you can finish those small changes this week as well?

window.Logger.log(window.Logger.LEVEL_DEBUG,
"while reading data for "+day+" from server, got nTrips = "+tripList.length);
console.log("About to hide 'Reading from server'");
console.log("About to show 'Reading from cache'");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these just indentation changes? You didn't actually change anything in the Timeline factory, did you?
We should probably have a standard for javascript indentation so that we don't run into issues like this that make it harder to review code.

What does your editor do? I don't mind following that..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I didn't change the factory. I'll reindent in the next pull request. I don't think I can make those changes this week. I'll try to do those next week.

Kris

Sent from my iPhone

On Apr 3, 2016, at 10:17, shankari notifications@github.com wrote:

In www/js/diary/services.js:

  •        $ionicPopup.alert({template: JSON.stringify(error)})
    
  •            .then(function(res) {console.log("finished showing alert");});
    
  •    });
    

- };

  • timeline.updateFromServer = function(day, foundFn, notFoundFn) {
  •    console.log("About to show 'Reading from server'");
    
  •    $ionicLoading.show({
    
  •          template: 'Reading from server...'
    
  •    });
    
  •    CommHelper.getTimelineForDay(day, function(response) {
    
  •       var tripList = response.timeline;
    
  •       window.Logger.log(window.Logger.LEVEL_DEBUG,
    
  •            "while reading data for "+day+" from server, got nTrips = "+tripList.length);
    
  •       console.log("About to hide 'Reading from server'");
    
  •  console.log("About to show 'Reading from cache'");
    
    Are these just indentation changes? You didn't actually change anything in the Timeline factory, did you?
    We should probably have a standard for javascript indentation so that we don't run into issues like this that make it harder to review code.

What does your editor do? I don't mind following that..


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@krisma
Copy link
Author

krisma commented Apr 11, 2016

simulator screen shot apr 10 2016 8 55 29 pm

Items on list NOT implemented:
Day of week on datepicker object. The format doesn't matter, since I can only change from MMDDYY or any other format, but can't add day of week to that.
For earlier or later thing, I didn't test because the data I have doesn't include trips that are common but different from common most frequent hour. I gave it some offset to test and it worked.But not tested with real data.

@shankari
Copy link
Contributor

Awesome!
Will merge soon and release early next week after I've had a chance to do
some testing.

Shankari

On Sun, Apr 10, 2016 at 10:57 PM, krisma notifications@github.com wrote:

[image: simulator screen shot apr 10 2016 8 55 29 pm]
https://cloud.githubusercontent.com/assets/6029441/14416419/cd4a490e-ff5e-11e5-8d90-4975b55c09c1.png

Items on list NOT implemented:
Day of week on datepicker object. The format doesn't matter, since I can
only change from MMDDYY or any other format, but can't add day of week to
that.
For earlier or later thing, I didn't test because the data I have doesn't
include trips that are common but different from common most frequent hour.
I gave it some offset to test and it worked.But not tested with real data.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#29 (comment)

@shankari shankari merged commit 57c468e into e-mission:master Apr 11, 2016
@krisma krisma deleted the mapBranch branch July 5, 2016 16:00
shankari added a commit that referenced this pull request Apr 23, 2020
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

Successfully merging this pull request may close these issues.

3 participants