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

Filter destinations in the map sidebar #941

Conversation

flibbertigibbet
Copy link
Contributor

@flibbertigibbet flibbertigibbet commented Dec 12, 2017

Overview

Implement a dropdown destinations filter for the map sidebar. Continue to use a button bar for filtering on the homepage, and switch between the two as appropriate.

Demo

no_events_isochrone

Notes

The prototype shows the word 'Filter' at the top of the filter bar, instead of the current selection. That is not typical drop-down behavior, and gives no clear benefit; there will always be a selection, as 'All' is the default. A standard select element is in use here.

As visible above, destinations that do not match the filter currently have a lower opacity, as do those outside the isochrone bounds. #937 is to not display locations that do not match the filter at all.

Created #940 to separate place list header and content reloading. Adding the filter bar has changed interaction such that the header shouldn't disappear on list reload; for now, it reloads whenever the list does.

Testing Instructions

  • vagrant ssh app
  • cd /opt/app/src && npm run gulp-development
  • Ensure service worker cache is cleared
  • Filtering on home and map pages should behave as expected
  • Navigating between home and map pages should switch between the two filter types

Checklist

  • No gulp lint warnings
  • Gulp tests pass

Closes #933
Closes #937

Create partial for destinations filter button bar and precompile it.
Also pass parameter whether on home page or not.
Also precompile destinations list template.
Map page dropdown functionality not implemented yet.
Implement filtering destinations via select on map page.
Fixes some inconsistencies and excess events around filtering as well.
Copy link
Contributor

@lederer lederer left a comment

Choose a reason for hiding this comment

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

Works well. Thanks for catching the silly default "Filter" label. I'll do some polish in a follow-up PR.

@KlaasH
Copy link
Contributor

KlaasH commented Dec 13, 2017

The interaction between this category filtering and the existing distance filtering for destinations seems to be causing some trouble. If I load destinations within a 15-minute bike ride of the office, it initially works fine (shows Fairmount Waterworks and Independence Seaport Museum), but if I select a category from the filter droplist, it loads all the desinations matching that filter, even if they're outside the given range. Then when I change it back to "All" it shows all destinations.

@flibbertigibbet
Copy link
Contributor Author

Hmm, good find @KlaasH. @lederer should the explore view list be filtered for both?

@lederer
Copy link
Contributor

lederer commented Dec 13, 2017

Yup. The list should be filtered by both the travelshed and the category.

Nice catch @KlaasH. Glad this PR had two reviewers. 😳

When filtering destinations on map sidebar, limit to those within isochrone (if present),
as well as filtering by selected category.
@flibbertigibbet
Copy link
Contributor Author

Updated filtering; ready for another look. Also added map marker category filtering to also close #937.

Copy link
Contributor

@KlaasH KlaasH left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

Just two small cleanup notes below.

@@ -401,6 +415,8 @@ CAC.Control.Explore = (function (_, $, Geocoder, MapTemplates, HomeTemplates, Ro

// now places list has been updated, go fetch the travel time
// from the new origin to each place

// TODO: cache travel times for last origin/routing params set?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, but it occurred to me that with the new filters, requeries will be fired off more frequently and often times will be unnecessary. I'll go make an issue and note it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// filter drop-down button event handler
$(options.selectors.filterToggle).on('change', function(e) {
e.preventDefault();
if (!e.target.selectedOptions || ! e.target.selectedOptions.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after ! looks odd, especially right after one with no space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that was accidental. Good catch.

Filter explore mode map markers by destination category.
Only show markers for destinations that match the selected category.
Markers in the category but outside the isochrone still show, with reduced opacity.

Closes azavea#937.
@flibbertigibbet flibbertigibbet force-pushed the feature/kak/filter-map-destinations#933 branch from 3129200 to c2ad095 Compare December 14, 2017 19:46
@flibbertigibbet flibbertigibbet merged commit 9c19705 into azavea:develop Dec 14, 2017
@flibbertigibbet flibbertigibbet deleted the feature/kak/filter-map-destinations#933 branch December 14, 2017 20:39
@lederer lederer mentioned this pull request Dec 19, 2017
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.

4 participants