Skip to content

chore: updated lng to lon#133

Merged
RubenSmn merged 1 commit intoGreenstand:mainfrom
muhon9:update-lng-to-lon
Nov 12, 2022
Merged

chore: updated lng to lon#133
RubenSmn merged 1 commit intoGreenstand:mainfrom
muhon9:update-lng-to-lon

Conversation

@muhon9
Copy link
Contributor

@muhon9 muhon9 commented Nov 12, 2022

Description

Updated all lng to lon for consistency

Issue(s) addressed

Resolves #127

Copy link
Member

@RubenSmn RubenSmn left a comment

Choose a reason for hiding this comment

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

Great work!

@RubenSmn RubenSmn merged commit 8b85fab into Greenstand:main Nov 12, 2022
Copy link
Collaborator

@dadiorchen dadiorchen left a comment

Choose a reason for hiding this comment

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

@RubenSmn please test this in the sandbox, there might be problem if the server side is returning lon rather then lng

const zoom_level = this.map.getZoom()
const res = await this.requester.request({
url: `${this.apiServerUrl}nearest?zoom_level=${zoom_level}&lat=${center.lat}&lng=${center.lng}`,
url: `${this.apiServerUrl}nearest?zoom_level=${zoom_level}&lat=${center.lat}&lon=${center.lon}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RubenSmn seems this requirs the change on API side too?

Copy link
Member

Choose a reason for hiding this comment

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

You are right @dadiorchen this causes an issue.

Copy link
Member

@RubenSmn RubenSmn Nov 14, 2022

Choose a reason for hiding this comment

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

The problem is also that the center does return a lng, this is a leaflet thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, we can do this way: revert this PR and re-create it, then let's go to the API side and finish the correction, then when can merge this PR, overwise, this PR will cause problem, right?

Copy link
Member

Choose a reason for hiding this comment

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

Could we update the API so that this will be fixed?

Copy link
Member

Choose a reason for hiding this comment

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

I created a PR for the API Greenstand/treetracker-web-map-api#353

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, I'll take a look, but I vaguely remember it's the PostgreSQL return lon so we align with it's convention maybe, you can try this:

select region_id id, ST_ASGeoJson(centroid) centroid from active_tree_region, region 
where region.id = active_tree_region.region_id

To check the return json from postgresql

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe there are some postgresql function can do some traslation

Copy link
Member

Choose a reason for hiding this comment

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

@dadiorchen just checked and the test is passing!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dadiorchen @RubenSmn is there anything i have to do?

@github-actions
Copy link

🎉 This PR is included in version 2.5.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

Update lng to lon for consistancy

3 participants

Comments