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.

Sign up for GitHub

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

Jump to bottom

Compare View Revamp: wire up maps and modification popovers #2154

Merged
merged 4 commits into from
Aug 21, 2017
Merged

Compare View Revamp: wire up maps and modification popovers #2154

merged 4 commits into from
Aug 21, 2017

Conversation

arottersman
Copy link

@arottersman arottersman commented Aug 15, 2017 •
edited
Loading

Overview

  • Add map views to each scenario column in the new compare view
  • Add modifications popovers to each map

Connects #2071

Demo

screen shot 2017-08-15 at 4 38 54 pm

screen shot 2017-08-15 at 4 40 08 pm

Notes

@jfrankl, two things:

  • If there's no modifications on the scenario, I opted not to show any popover. Let me know if you'd like to see one that says "No modifications" or something.
  • In the wireframe, you've dropped the mod type label "Land Cover" or "Conservation Practices". Was this intentional?

screen shot 2017-08-15 at 5 17 50 pm

I kept them in for now.

Testing Instructions

  • Pull and ./scripts/bundle.sh
  • In the compare view for a TR-55 project with some modified scenarios...
  • Confirm the mini-maps' extents are the modifications, if they exist.
  • Hover over a map with mods (or tap on mobile) and confirm a popover appears with the areas for each type

Alice Rottersman added 2 commits August 15, 2017 13:33
Wire maps into new compare view
8f12d16 
- Create a map view for each scenario
- Set extent of map view to that scenarios modifications if they exist,
  or the aoi
Add modifications popover to compare maps
45c1b0d 
- if a scenario has modifications, add a popover over its map to display them
kellyi
kellyi reviewed Aug 16, 2017
View reviewed changes
src/mmw/js/src/compare/templates/compareModificationsPopover.html
@@ -0,0 +1,23 @@
{% macro modificationsTable(caption, modifications) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Didn't know this was possible.

Copy link
Author

Choose a reason for hiding this comment

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

Same! I stole this from the original implementation of the mod table. Nunjunks has a quite a few tricks up its sleeves

@kellyi
Copy link
Contributor

kellyi commented Aug 16, 2017

This is working really well! Here's a project with a scenario including all modifications types:

screen shot 2017-08-16 at 10 10 21 am

@kellyi
Copy link
Contributor

kellyi commented Aug 16, 2017

Here's one with a modification larger than the initial AOI; map extent's changed correctly:

screen shot 2017-08-16 at 10 18 03 am

@kellyi
Copy link
Contributor

kellyi commented Aug 16, 2017

I think this is due to something in the compare view's CSS rules but on Safari the font weight for the precipitation slider and some of the other text seems to drop slightly yet notice-ably when one of the popovers is open:

screen shot 2017-08-16 at 10 20 42 am

screen shot 2017-08-16 at 10 20 54 am

kellyi
kellyi approved these changes Aug 16, 2017
View reviewed changes
Copy link
Contributor

@kellyi kellyi left a comment

Choose a reason for hiding this comment

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

This looks good to me! Tested it out in Chrome, FF, Safari, and IE 11 and it's working well in each.

There's a slight quirk with the font-weight changing in Safari on opening one of the popovers; not sure whether it's related to something changed here or something already in the new compare view CSS (see similar quirks here #2148 (comment)).

If that's fixable here it may be worth doing; otherwise we should make a new card to track compare view style quirks which need fixing.

@kellyi kellyi assigned arottersman and unassigned kellyi Aug 16, 2017
@arottersman
Copy link
Author

Thanks, @kellyi! I went over this with Jeff. I'll be adding a commit that adds some placeholder text to popovers for scenarios that can't/don't have modifications with the intention that someday:

  • 100% Forest will have some description of itself
  • Current Conditions will have some description of itself
  • Regular scenarios without modifications will say No modifications + something like "to add modifications...."

I'll also look into that safari styling quirk.

arottersman
arottersman commented Aug 16, 2017
View reviewed changes
src/mmw/js/src/compare/views.js Outdated
var title = this.model.get('name'),
text;

if (this.model.get('is_pre_columbian')) {
Copy link
Author

Choose a reason for hiding this comment

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

This could be easily done in the template as well, but I figured it was easier to debug if we kept the logic here.

Copy link
Author

Choose a reason for hiding this comment

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

Actually on second thought I'm going to move it to the template.

@arottersman
Copy link
Author

  • Added a commit to fix the safari quirk
  • Added additional popovers
    screen shot 2017-08-16 at 1 50 36 pm
    screen shot 2017-08-16 at 1 50 46 pm
    screen shot 2017-08-16 at 1 50 31 pm

@arottersman
Copy link
Author

@kellyi, before I merge, mind giving this a final look with the new popovers?

@kellyi
Copy link
Contributor

kellyi commented Aug 21, 2017

Sure, happy to test it out again!

@kellyi
Copy link
Contributor

kellyi commented Aug 21, 2017

+1, working well! Here's the 100% forest cover popover on IE11:

screen shot 2017-08-21 at 11 39 44 am

Also working in FF, Safari, and Chrome.

Alice Rottersman added 2 commits August 21, 2017 13:52
Show popover descriptions for scenarios with no modifications
31925b9 
- Add a new popover view to show if a scenario can't or doesn't have
  modifications
 - Current Conditions and 100% Forest Cover explain what they are
 - Regular Scenarios without modifications, describe how to add modifications
Set compare view font-smoothing
cc6656f 
- Safari was updating the font-smoothing when the modification popovers opened.
  Set it explicitly so it doesn't flicker
@arottersman
Copy link
Author

Thanks for all the testing and review! Squashed the fixup. Will merge this once the build completes.

@arottersman arottersman merged commit e860700 into develop Aug 21, 2017
@arottersman arottersman deleted the arr/tiny-compare-maps branch August 21, 2017 19:06
@rajadain rajadain mentioned this pull request Oct 16, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kellyi kellyi kellyi approved these changes

Assignees

@arottersman arottersman

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arottersman @kellyi @hectcastro