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

Update to use mapbox-gl and d3 v7. #330

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

epaulson
Copy link
Collaborator

This is a big update that switches to mapbox-gl instead of mapbox 0.6.7, which is nearly 9 years out of date. This restores map tiles to the background.

This also updates to d3 version 7, instead of d3 version 3.

I use a bit of turf.js for some calculations. This includes a smaller bundle of 4 functions created with browersify instead of including the full 500K library. (Using a custom bundle is recommended by turf.js)

This includes a new 'maxLat' setting that can be used by metadata.json - if it's present, the geojson is clipped to that latitude, which helps reduce distortion from the mercator projection. The europe-1914 and europe-1938 maps now use it, along with north-america and canada.

chicago-parks is disabled from now. For whatever, d3 version 6 and 7 cannot correctly display that geojson - d3 v5 handles it just fine, as does mapshaper.org I can reproduce the problem with a super-basic d3 only example, so I will open an issue with d3 and figure out if there's a bug in d3 or something wrong with the chicago-parks geojson

The main remaining problem is that when the aspect ratio gets too out of bounds - say you resize your browser window to be tall but narrow - the geojson svg is no longer properly placed relative to the map tiles. In looking at it, I don't see how this bug wasn't also present in the existing codebase, if map tiles were still working- the SVG container is set to fit the entire map, but because of the logo on the side and the vertical mercator distortion, the map that we draw to fill up the entire background simply can't fit right with the smaller SVG. It is most noticeable on the russia and asia maps, which are very wide by also go very far north. Because I'm pretty sure the bug was there before, I'm opening a PR now, and we can figure out how to fix it later - set some minimum size and turn on scrollbars if necessary, or only draw the map under the SVG, or clip the geojson, or let a map disable tiles in its metadata.json - russia and asia are quite playable without the tiles.

…x for a few functions. add max latitude clipping for 4 maps. disable chicago-parks for now, d3 cannot process it.
@epaulson
Copy link
Collaborator Author

epaulson commented Jan 8, 2022

Hi @fnogatz - Happy New Year! Any thoughts about this one?

@fnogatz fnogatz self-assigned this Jan 11, 2022
public/css/styles.less Show resolved Hide resolved
public/index.html Outdated Show resolved Hide resolved
public/index.html Outdated Show resolved Hide resolved
public/js/scripts.js Show resolved Hide resolved
@fnogatz
Copy link
Member

fnogatz commented Jan 11, 2022

Hi @epaulson, thank you very much for all the effort you put into this! I reviewed your changes. Would you mind to take into account these suggestions?

@fnogatz
Copy link
Member

fnogatz commented Jan 11, 2022

There are some legacy code fragments in public/js/scripts.js. I would suggest first merging your changes and then taking some time to clean up this file.

@fnogatz
Copy link
Member

fnogatz commented Jan 11, 2022

I use a bit of turf.js for some calculations. This includes a smaller bundle of 4 functions created with browersify instead of including the full 500K library. (Using a custom bundle is recommended by turf.js)

I would like to have this repository self-contained, so we can reproduce how to create the custom bundle in the future. What about adding browserify and the turf modules as dependencies to package.json and include the bundling step in the npm run build script?

@epaulson
Copy link
Collaborator Author

I have updated the PR to have local copies of d3 and mapbox-gl. I did also notice that there was some unused code in js/lib, and I thought we'd leave that for a future PR to remove it.

I'd also rather put the new turf.js build into a future PR, mostly because I don't really know how to use the JS build tools yet! But yeah, we should do a real browserify step in the build rather than leaving everything static.

For the record, this is what I'm using for browserify to build the turf bundle

module.exports = {
    bboxClip: require('@turf/bbox-clip').default,
	lineString: require('@turf/helpers').lineString,
	featureCollection: require('@turf/helpers').featureCollection,
	featureEach: require('@turf/meta').featureEach
};

@fnogatz fnogatz merged commit b92acc4 into codeforgermany:main Jan 18, 2022
@fnogatz
Copy link
Member

fnogatz commented Jan 18, 2022

Aaaaand it's live: https://click-that-hood.com/

Thank you very much, @epaulson! 👏

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.

2 participants