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

bugfix/17086-mapline-in-non-topojson #18130

Closed

Conversation

kamil-musialowski
Copy link
Contributor

@kamil-musialowski kamil-musialowski commented Dec 12, 2022

Fixed #17086, mapline didn't work with lon/lat coordinates on non-TopoJSON maps

@highsoft-bot
Copy link
Collaborator

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
highmaps.js 125.4 kB
381.3 kB
125.4 kB
381.5 kB
39 B
244 B

@highsoft-bot
Copy link
Collaborator

Visual test results - Differences found

Found 1 diffing sample(s). Please review the differences.

@kamil-musialowski kamil-musialowski marked this pull request as draft December 12, 2022 16:48
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

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

Hi, this is how geometries work with the legacy GeoJSON files. Not because they are GeoJSON, but because they have the crs specifying the projection and the hc-transform attribute that defines how geometries should be translated. So basically, all geometries in the legacy files are pre-projected.

The Point geometry is an exception because I made it work identically to the lon and lat point options. So if there is a bug, that is it. But if no-one is reporting it, I don't think we should waste more time on supporting the legacy maps.

In the OP demo, you can see that the fix only works with LineString, but not MultiLineString, Polygon etc.

Please revert the added code in this PR. But the question is if we should add some heads-up in the docs under the geometry option?

@KacperMadej
Copy link

@TorsteinHonsi

But the question is if we should add some heads-up in the docs under the geometry option?

That would be very nice. The current description mentions only geoJSON (by the way, for consistent should it be GeoJSON?). Should it also contain info about the geometry of 'Point' type working differently? or is that just a small inconsistency waiting to be addressed when reported?

@TorsteinHonsi
Copy link
Collaborator

That would be very nice. The current description mentions only geoJSON (by the way, for consistent should it be GeoJSON?).

Yes, should be GeoJSON. The description is technically correct. TopoJSON is missing because it unpacks into GeoJSON, but should probably be mentioned nonetheless.

The destinction in the behaviour is not between GeoJSON and TopoJSON per se, but between sources that are pre-projected (like our legacy GeoJSON files are), and those that are not (like our TopoJSON files are). So I guess this should be described.

Should it also contain info about the geometry of 'Point' type working differently? or is that just a small inconsistency waiting to be addressed when reported?

I think the description of the mappoint.data.geometry is correct, so no need to elaborate there for the time being.

@kamil-musialowski
Copy link
Contributor Author

Moved to #18148

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.

mapline geometry-based data on a non-TopoJSON
4 participants