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

Upgrade ol5 #8

Merged
merged 8 commits into from
Aug 15, 2018
Merged

Upgrade ol5 #8

merged 8 commits into from
Aug 15, 2018

Conversation

dnlkoch
Copy link
Member

@dnlkoch dnlkoch commented Aug 14, 2018

This updates the ol-util classes to make use of the current version of ol (5.1.3).

In addition eslint is added.

Copy link
Member

@ahennr ahennr left a comment

Choose a reason for hiding this comment

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

Nice work @dnlkoch

Just a minor question regarding test of MeasureUtil

@@ -38,8 +38,8 @@ describe('MeasureUtil', () => {
const expectedShortLength = MeasureUtil.getLength(shortLine, map);
const expectedLongLength = MeasureUtil.getLength(longLine, map);

expect(expectedShortLength).toEqual(99.99999999669033);
expect(expectedLongLength).toEqual(100545.83533277796);
expect(expectedShortLength).toEqual(99.88824008937313);
Copy link
Member

Choose a reason for hiding this comment

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

Why did these values change?

Copy link
Member

@hwbllmnn hwbllmnn Aug 14, 2018

Choose a reason for hiding this comment

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

Also, floats/doubles should not be checked for equality, but compared using a delta (0.1 + 0.2 !== 0.3).

Copy link
Member Author

Choose a reason for hiding this comment

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

The default sphere radius taken into account for length/area calculation in ol is set to 6371008.8 whereas the previous implementation in ol-util used the non-default value 6378137 (that is the value equal to the semi-major axis of the WGS84 ellipsoid).

If needed the value may be passed to the corresponding util methods, but in here I would like to make use of the same radius as used in ol per default.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hwbllmnn Yes, absolutely! Fixed (and will be fixed for all other occurrences in a separate PR).

@dnlkoch dnlkoch merged commit 5f658a2 into terrestris:master Aug 15, 2018
@dnlkoch dnlkoch deleted the upgrade-ol5 branch August 15, 2018 06:36
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