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

696 click ordering #783

Merged
merged 1 commit into from
Feb 8, 2019
Merged

696 click ordering #783

merged 1 commit into from
Feb 8, 2019

Conversation

allthesignals
Copy link
Collaborator

@allthesignals allthesignals commented Feb 7, 2019

Fixes a regression with tooltip templating and addresses #696 by upgrading ember-mapbox-composer which includes a better approach to handling hover events for overlapping layers. Needs some review to see if the behavior has improved.

@hannahkates if you can find specific issues maybe GIF them or show me ways to manually test this.

Closes #696 Closes #785 Closes #786 Closes #785

Thanks!

@allthesignals allthesignals requested review from a user and hannahkates February 7, 2019 01:27
@ghost ghost assigned allthesignals Feb 7, 2019
@ghost ghost added the in progress label Feb 7, 2019
@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #783 into develop will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #783     +/-   ##
==========================================
+ Coverage    59.07%   59.68%   +0.6%     
==========================================
  Files           71       71             
  Lines          694      692      -2     
==========================================
+ Hits           410      413      +3     
+ Misses         284      279      -5
Impacted Files Coverage Δ
app/routes/application.js 96.29% <ø> (ø) ⬆️
app/components/tooltip-renderer.js 100% <100%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72db160...7e50f28. Read the comment docs.

@ghost
Copy link

ghost commented Feb 7, 2019

Looks good, and I approved the percy diffs. But this is waiting on the merge & tag of that last update to ember-mapbox-composer, right? And then another change here to bump the addone version one more time ?

@allthesignals
Copy link
Collaborator Author

Looks good, and I approved the percy diffs. But this is waiting on the merge & tag of that last update to ember-mapbox-composer, right? And then another change here to bump the addone version one more time ?

Not sure I understand. I think these changes include a version bump to grab the necessary version of mapbox composer.

@chriswhong
Copy link
Contributor

Closes #696

Copy link
Contributor

@hannahkates hannahkates left a comment

Choose a reason for hiding this comment

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

I've observed two issues in this PR preview. #786 and #785. #786 is especially high priority to fix before demo.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

looks GREAT !

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #783 into develop will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #783     +/-   ##
==========================================
+ Coverage    59.07%   59.68%   +0.6%     
==========================================
  Files           71       71             
  Lines          694      692      -2     
==========================================
+ Hits           410      413      +3     
+ Misses         284      279      -5
Impacted Files Coverage Δ
app/routes/application.js 96.29% <ø> (ø) ⬆️
app/components/tooltip-renderer.js 100% <100%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72db160...7e50f28. Read the comment docs.

2 similar comments
@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #783 into develop will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #783     +/-   ##
==========================================
+ Coverage    59.07%   59.68%   +0.6%     
==========================================
  Files           71       71             
  Lines          694      692      -2     
==========================================
+ Hits           410      413      +3     
+ Misses         284      279      -5
Impacted Files Coverage Δ
app/routes/application.js 96.29% <ø> (ø) ⬆️
app/components/tooltip-renderer.js 100% <100%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72db160...7e50f28. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #783 into develop will increase coverage by 0.6%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop     #783     +/-   ##
==========================================
+ Coverage    59.07%   59.68%   +0.6%     
==========================================
  Files           71       71             
  Lines          694      692      -2     
==========================================
+ Hits           410      413      +3     
+ Misses         284      279      -5
Impacted Files Coverage Δ
app/routes/application.js 96.29% <ø> (ø) ⬆️
app/components/tooltip-renderer.js 100% <100%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72db160...7e50f28. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #783 into develop will increase coverage by 0.59%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop    #783      +/-   ##
==========================================
+ Coverage    60.11%   60.7%   +0.59%     
==========================================
  Files           72      72              
  Lines          707     705       -2     
==========================================
+ Hits           425     428       +3     
+ Misses         282     277       -5
Impacted Files Coverage Δ
app/routes/application.js 96.29% <ø> (ø) ⬆️
app/components/tooltip-renderer.js 100% <100%> (+100%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8327b50...afb6b72. Read the comment docs.

@hannahkates
Copy link
Contributor

Hmm something a little weird is happening where lots now have multiple lot number labels Diff example: https://percy.io/NYC-Planning-Labs/zola/builds/1419006/view/93862159/1280?browser=firefox&mode=diff
(Percy is awesome)

@hannahkates
Copy link
Contributor

hannahkates commented Feb 7, 2019

Oh wonderful, just realized the supporting layers are (mostly) fixed now! Only remaining issue I'm seeing from #786 is that the points (e-designations and landmark points) aren't layered on top of tax lots, so you can't hover or click on them when zoomed in. #785 looks fully resolved.

@allthesignals
Copy link
Collaborator Author

Oh wonderful, just realized the supporting layers are (mostly) fixed now! Only remaining issue I'm seeing from #786 is that the points (e-designations and landmark points) aren't layered on top of tax lots, so you can't hover or click on them when zoomed in. #785 looks fully resolved.

You can if you turn off tax lots and they seem to work fine. In the original issue, @andycochran mentioned that "tax lots should ALWAYS be the topmost layer" - so I'm confused by this.

@allthesignals
Copy link
Collaborator Author

allthesignals commented Feb 8, 2019

Hmm something a little weird is happening where lots now have multiple lot number labels Diff example: https://percy.io/NYC-Planning-Labs/zola/builds/1419006/view/93862159/1280?browser=firefox&mode=diff
(Percy is awesome)

Yeah - I wouldn't read too much into the diffing here. The maps generate PNG images (which are there screenshottable by Percy), so there'll almost always be weird little artifacts here and there. It's mostly there to catch big things - so if you see a lot of red, that's usual a sign that something's up. In this case, it's probably fine - you should just verify that it's working in the branch preview.

@ghost ghost force-pushed the 696-click-ordering branch from 7e50f28 to 0690a5d Compare February 8, 2019 15:51
@ghost ghost self-assigned this Feb 8, 2019
Reorder default layergroup state to put circle features, then tax lots
on top

Bump mapbox-composer fix to address 696

Enable test for coverage
@ghost ghost force-pushed the 696-click-ordering branch from 0690a5d to afb6b72 Compare February 8, 2019 15:53
@ghost
Copy link

ghost commented Feb 8, 2019

Hmm something a little weird is happening where lots now have multiple lot number labels Diff example: https://percy.io/NYC-Planning-Labs/zola/builds/1419006/view/93862159/1280?browser=firefox&mode=diff
(Percy is awesome)

Yeah - I wouldn't read too much into the diffing here. The maps generate PNG images (which are there screenshottable by Percy), so there'll almost always be weird little artifacts here and there. It's mostly there to catch big things - so if you see a lot of red, that's usual a sign that something's up. In this case, it's probably fine - you should just verify that it's working in the branch preview.

Unfortunately, this is not just a Percy artifact... I'm seeing it in staging right now. That being said I think we should merge this branch to resolve all these issues, and we can address the multiple lot IDs separately.
screen shot 2019-02-08 at 10 59 55 am

@hannahkates
Copy link
Contributor

Oh wonderful, just realized the supporting layers are (mostly) fixed now! Only remaining issue I'm seeing from #786 is that the points (e-designations and landmark points) aren't layered on top of tax lots, so you can't hover or click on them when zoomed in. #785 looks fully resolved.

You can if you turn off tax lots and they seem to work fine. In the original issue, @andycochran mentioned that "tax lots should ALWAYS be the topmost layer" - so I'm confused by this.

@allthesignals Andy's comment did say tax lots on top, but after we had discussed this more, we agreed points should be on top followed by tax lots. I had updated the title and description of the issue accordingly, but I think this raises an important point that we need a better way of clearly indicating the final decision on an issue vs. preserving discussion leading up to it. I feel like the description is the best place for that, so you don't have to go digging through comments, but we should discuss and agree on the preferred method.

@hannahkates hannahkates merged commit 15f7fff into develop Feb 8, 2019
@ghost ghost removed the in progress label Feb 8, 2019
@hannahkates hannahkates deleted the 696-click-ordering branch May 6, 2019 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants