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

BiG-CZ Map Popups #2173

Merged
merged 3 commits into from
Aug 23, 2017
Merged

BiG-CZ Map Popups #2173

merged 3 commits into from
Aug 23, 2017

Conversation

arottersman
Copy link

Overview

Adds popovers to the BiG-CZ results we display on the map (Currently CINERGI and WDC).

Wireframe
screen shot 2017-08-22 at 11 18 33 am

Connects #2136

Demo

screen shot 2017-08-22 at 11 10 47 am
screen shot 2017-08-22 at 11 11 20 am

th8cajmzhy

Notes

  1. The wireframes don't reflect the actual data for the catalogs, so I just reused the ResultView for the popups while we determine what they should contain
  2. The interaction for opening a CINERGI popup is weird because you have to click the skinny boundary line. Clicking the inside of the boundaries would also be bad though; boundaries overlap.
  3. I didn't match the wireframe "detail" button exactly because setting it to ui-secondary seemed much to dark on the white background
  4. Refactored the enter-detail-mode triggering to allow the popup access

Testing Instructions

  • Pull and bundle. There are style changes, make sure the sass compiles.
  • Get results for the three tabs by searching something like water
    • Confirm you can still open the details from the sidebar from all three catalogs
    • Confirm if you click on a Cinergi map boundary, its corresponding popup opens and you can use its details button
    • Confirm the same for the WDC points

Alice Rottersman added 2 commits August 21, 2017 13:36
- Render details of results on popup
   - Currently reuses ResultView while we determine what is appropriate
     for the popup content
- Add "Details" button to popup to display details in sidebar
    - To accomplish, some refactoring:
      * Move triggering of details view out from the views
    and onto the model. Now each result has a `show_detail` boolean
    that the ResultsCollection manages. The catalog listens for changes
    on the collection to set a `detail_result` attribute, which in turn
    is listened to by the sidebar view.
      * This way the parent-view orphaned
    popover can trigger the detail in addition to the main result list view
- Apply custom leaflet popup styling
@arottersman
Copy link
Author

/cc @jfrankl for style review

// track of which shape came from where
return _.map(filteredResults, function(r) {
if (r.geom) {
r.geom.properties = {
Copy link
Author

Choose a reason for hiding this comment

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

We could do this on the backend as well, but if we end up exposing the API, our users might not want the duplicate data clogging up the response.

Copy link
Member

Choose a reason for hiding this comment

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

If we ever did #1950 it would assist with this. For now I think this is fine.

@jfrankl
Copy link
Contributor

jfrankl commented Aug 22, 2017

When the user clicks on "details" in the popup, the popup should close. Other than that, this is good to go!

FYI, @ajrobbins and I have another concept for the bounding box interaction. We'll write that up in a separate issue.

@rajadain
Copy link
Member

Waiting for a fixup to come in for the hover issue discussed IRL. Otherwise:

  • I like the popups, and agree that reusing the Result View for this purpose. We should remove the hover interaction of the ResultView in this case.
  • Clicking on the edge of CINERGI results feels a little odd. Good enough for now, but we discussed the possibility of sorting the items on the map to keep the smallest at the top, so you could click on an inside area and it will select the smallest / most locally relevant one. This can be attempted later.
  • When we go into the details view, the map element should be highlighted until the user goes back. I remember there being some discussion of hiding all other map elements, keeping only the selected one, in the details view, but highlighting the selected one seems like an easy thing to do at this stage.

* Highlight geom for result with an open popup
* Highlight geom for result with open detail panel
* Close popup on detail open
@arottersman
Copy link
Author

arottersman commented Aug 23, 2017

Pushed a commit to modify the highlight behavior.

One quirk is in the details page you can still open additional popups, and they will highlight. That may or may not be desired.

@rajadain
Copy link
Member

I created #2181 for all the hover issues, even some found herein, so I think this is good to merge. Just going to take a quick peek over the code now.

@arottersman
Copy link
Author

Thanks, those are good ideas for the hovering. I think it might take some time to get the interactions right, so I'm all for deferring.

Copy link
Member

@rajadain rajadain left a comment

Choose a reason for hiding this comment

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

+1 tested. This looks good enough, and works well. We're still evolving our approach to the map / sidebar interaction, and will follow up with #2181. Nice work.

// track of which shape came from where
return _.map(filteredResults, function(r) {
if (r.geom) {
r.geom.properties = {
Copy link
Member

Choose a reason for hiding this comment

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

If we ever did #1950 it would assist with this. For now I think this is fine.

@rajadain rajadain assigned arottersman and unassigned rajadain Aug 23, 2017
@arottersman
Copy link
Author

Thanks! Going to merge.

@arottersman arottersman merged commit 939943b into develop Aug 23, 2017
@arottersman arottersman deleted the arr/bigcz-map-popup branch August 23, 2017 20:29
@rajadain rajadain mentioned this pull request Oct 16, 2017
@rajadain rajadain added the BigCZ label Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants