Feature/time to places #719
Conversation
There was a problem hiding this comment.
My main issue is that I think this should go in the Explore controller rather than Directions. I don't see where it's using anything that's unique to Directions, and it seems like it's making some things clunkier (or at least incongruous, e.g. where the Home controller activates the Explore tab then calls some methods from the Directions controller because Explore is where the results of the methods actually show up).
Re "remove that Django endpoint and just query OTP directly", that sounds good to me, but we should get confirmation from @lederer whether filtering to accessible places is in or out, since it is shown in the Marvelapp (see item 2 of issue #713).
| }); | ||
| } | ||
|
|
||
| var getNearbyPlaces = _.throttle(function() { // jshint ignore:line |
There was a problem hiding this comment.
Is there a reason this functionality needs to live with the Directions controller? I would think it would fit much more naturally in the Explore controller (which is where the code that used to do this stuff lived).
| var date = UserPreferences.getPreference('dateTime'); | ||
| date = date ? moment.unix(date) : moment(); // default to now | ||
|
|
||
| var otpOptions = getOtpOptions(); |
| modeOptionsControl.setMode(UserPreferences.getPreference('mode')); | ||
| showHideNeedWheelsBanner(); | ||
| setActiveTab(); | ||
| exploreControl.setFromUserPreferences(); |
There was a problem hiding this comment.
What was malfunctioning with the old sequence? Whatever works works, but the old way made more conceptual sense to me--setting the tab first so that things that are affected by that know how to behave, then the control components which possibly change things in ways that will inform the Directions and Explore controllers, then the controllers themselves (in either order, since they should be independent).
There was a problem hiding this comment.
There's some issues around ordering of events there. Setting the tab then fires the tab change handler, which would be the appropriate place to handle view change, but the settings may not yet be correct if setFromUserPreferences has not yet been called... but at least one setFromUserPreferences method expects the tab to have already been set.
I fiddled with several things before settling on something that worked, so not sure if this particular ordering change is actually still necessary or not.
There was a problem hiding this comment.
You're right, first doesn't make sense. Now I'm thinking last does. Explore and Directions will update based on user preferences but will only do their main thing (get directions or draw isochrone) if they're active. Then when setActiveTab runs if the tab hasn't changed nothing will happen but if the tab has changed the one being activated will then do its thing.
The problem with that setup is that if the URL change implies a tab switch from Explore to Directions or vice versa then both actions will fire, and the one we're moving from will be firing uselessly. Probably this is evidence that the triggering conditions are muddled and not quite right. I.e. the logic of how/what to trigger on options/origin/destination changes vs URL changes.
| } | ||
| } | ||
|
|
||
| if (key === 'origin') { |
There was a problem hiding this comment.
This would make more sense in the onTypeheadSelected of the controller that contains getNearbyPlaces. Both Directions and Explore already have onTypeaheadSelected methods.
There was a problem hiding this comment.
Put it here because the places need to be updated for either tab; in case user changes origin in directions tab, then switches back to explore.
There was a problem hiding this comment.
The onTypeahead functions all run whenever the typeahead sends its signals, so components can can keep their data in sync. I.e. directions and explore are already updating their local variables from the events, then deciding whether to do anything more based on whether they're active. So adding this to Explore's onTypeahead methods inside the if (key === 'origin') (which is currently missing from onTypeaheadSelected, which is a bug) but outside the tab check would work fine and would make more sense to me.
|
The reason for putting this in the directions controller is that it owns the rather complex trip plan method that is what gets called to find the travel time to each place. Relatedly, putting it there is what let me factor out the shared trip parameter setup business. Although these do appear on the explore tab, this also gets run for the home page display. It could perhaps get factored out into a separate common module, but if so, it would also need to own the trip plan business in the directions module now. |
|
@jbranigan Do you recall where we landed re "whether filtering to accessible places is in or out" (per @KlaasH's comment)? It seems odd that we'd allow the user to set a travel time limit, but then show results outside that limit. OTOH since I can't recall where we landed, for all I know I may have argued for the opposite. :/ |
| var planTrip = _.throttle(function() { // jshint ignore:line | ||
| if (!tabControl.isTabShowing(tabControl.TABS.DIRECTIONS)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is mostly redundant, but not entirely. setOptions is being called by a listener in the Home controller and is in turn calling planTrip without checking whether the tab is active. Probably putting the check in setOptions makes sense.
|
Re where to put it: the OTP options setup in Explore looks to be the same as that in Directions except for waypoints, which aren't relevant to place times. Probably the best thing would be for that to be factored out into some shared resource. Maybe A separate controller maybe makes the most sense. It would mean more lines of code overall, to create the new controller and set it up with the listeners and such that it needs, rather than having it piggyback on either of the existing ones. But it would probably be cleaner and easier to deal with. That being said, I also did a proof-of-concept version of putting it in Explore, which I put in a branch and PRed against this branch: flibbertigibbet#1 |
Delete old django templates unused after the redesign.
Keep IDs for place cards so they may be found again later.
Move OTP parameter building from user preferences to its own method.
Update Handlebars template for destinations, for redesign.
When origin updates, reload list of featured destinations to order by distance from origin.
On origin change.
Get travel times to destinations working again after rebase.
When populating travel time to each. Due to changes in URL update.
And clear when origin control cleared.
When logo clicked.
Refactor to use named selectors instead of strings.
Also fix times to places not loading on page refresh with origin in URL.
2c7e76e to
4a5b7a4
Compare
|
Superseded by #725, which is this branch with the changes from flibbertigibbet#1 cherry-picked in and merge conflicts fixed. |
Closes #557.
Display travel time to each destination in its card when a origin is set.
Recalculates on change to origin or to any trip options. Displays on home page and in explore tab.
Isochrone requests go to a special endpoint that bundles up OTP's response with a list of destinations filtered to be within the isochrone; however, we're no longer using those filtered locations, so if we're not bringing them back, we should remove that Django endpoint and just query OTP directly.
For efficiency and simplicity, I think that would be a good way to go. Otherwise we'd end up having to requery the travel times to each location and/or set up origin+destination distance caching to use whenever the isochrone scrubber moves.