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

fix: set the parcel coordinates for the browser #255

Merged
merged 3 commits into from Oct 7, 2022

Conversation

tnrdd
Copy link
Collaborator

@tnrdd tnrdd commented Oct 7, 2022

Description

The parcel coordinates to be passed to the spatial browser were not being set when the user fly to a parcel from the portfolio modal, this fixes it.

Issue

fixes #254

Checklist:

  • My commit message follows the Conventional Commits specification
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my code
  • My changes generate no new warnings
  • My PR is rebased off the most recent develop branch
  • My PR is opened against the develop branch

Alert Reviewers

@codynhat @gravenp

@@ -129,7 +131,7 @@ const portfolioQuery = gql`
contributionRate
}
coordinates {
pointTL {
pointTR {
Copy link
Member

Choose a reason for hiding this comment

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

We have received feedback about the zoom not going to the center of the parcel. Is this an easy fix? Maybe we need to get two corners and find the center point?

Copy link
Collaborator Author

@tnrdd tnrdd Oct 7, 2022

Choose a reason for hiding this comment

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

The problem is that when the sidebar is opened it shift right the parcel, it can be seen even when clicking on parcel, so to sort of center it an offset is added.
I think that to really center it accounting for all parcel sizes beside the offset we need to find the center point.
Does the coordinates array comes spatial ordered? If yes it should be easy to fix.

Copy link
Member

Choose a reason for hiding this comment

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

I think Nick's feedback focused on when you single-click on the map to start a land claim: the single yellow coordinate is offset to the right a bit (I assume because of the same panel shift). Seems like this single-coordinate centering should be pretty straight-forward? Lmk if you want to include that change here or if you want a separate ticket.

As for the parcel-level centering on zoom, that would definitely be nice to have too if possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Given that the offset constant was moved to the map in this PR I think we can include it here.

Copy link
Member

@gravenp gravenp 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. The latest build fixes the original issue of coordinates not populating in the browser when navigating through the dashboard and the secondary issue of centering the map on a single coordinate when starting a claim.

It doesn't center on an existing parcel based on its center point yet, but based on the open conversation that's expected.

If that is feasible to do, then let's use that method for both Profile/Dashboard navigation and when a user clicks any parcel from the map. Otherwise, I think that the adjustments that @tnrdd previously made do a fair enough job at bringing the user to the parcel.

@tnrdd
Copy link
Collaborator Author

tnrdd commented Oct 7, 2022

If that is feasible to do, then let's use that method for both Profile/Dashboard navigation and when a user clicks any parcel from the map. Otherwise, I think that the adjustments that @tnrdd previously made do a fair enough job at bringing the user to the parcel.

In the last push for the portfolio modal case the center point of the parcel selected is found and it should be nicely centered.
For the clicked parcel only the offset is applied just so the parcel doesn't move off screen, to use the center point in this case we would need to do additional queries or some blocking operations and change the zoom level (right now the zoom is not touched on parcel click).

@codynhat
Copy link
Member

codynhat commented Oct 7, 2022

LGTM!

Copy link
Member

@gravenp gravenp left a comment

Choose a reason for hiding this comment

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

Agreed. Latest push looks really good!

@codynhat codynhat merged commit 4c7b2cf into Geo-Web-Project:develop Oct 7, 2022
@tnrdd tnrdd deleted the fix/open-browser branch October 12, 2022 12:29
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.

None yet

3 participants