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

Support geoPath.digits(n) in geo projection components #1761

Closed
kachkaev opened this issue Oct 23, 2023 · 5 comments · Fixed by #1767
Closed

Support geoPath.digits(n) in geo projection components #1761

kachkaev opened this issue Oct 23, 2023 · 5 comments · Fixed by #1767

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Oct 23, 2023

When @visx/geo generates an SVG, paths end up containing more than 10 fractional digits. This increases page download size but is not practically useful. Fractional digits are often ‘random’, which means that transport-level compression cannot be very efficient for them.

Here is an example: https://airbnb.io/visx/geo-custom

Screenshot 2023-10-23 at 09 48 45

It turns out that d3-geo supports path.digits(n) since version 3.1.0 (see d3/d3-geo#272 for context). It’d be nice to add support for digits in @visx/geo, similar to how it’s already done for pointRadius:

  <CustomProjection<FeatureShape>
    {...miscProps}
    pointRadius={2}
+   digits={3}
  >

The result would be a much smaller SVG:

-<path d="M365.0728966344956,172.6175701076138L364.8094078304068,172.92693679404636L364.47832959211263,172.75727516334257ZM369.92342393578775,172.58797232220107L369.90552269562437..." fill="#f4448b" stroke="#252b7e" stroke-width="0.5" class="jsx-296077636"></path>
+<path d="M365.072,172.617.809,172.926.478,172.757.923,172.587.905..." fill="#f4448b" stroke="#252b7e" stroke-width="0.5" class="jsx-296077636"></path>

Here is the file we’ll need to change:

const path = geoPath().projection(currProjection);
if (pointRadius) path.pointRadius(pointRadius);

  const path = geoPath().projection(currProjection);

+ if (digits) path.pointRadius(digits);
  if (pointRadius) path.pointRadius(pointRadius);

We’ll also need to bump d3-geo from ^1.11.3 to ^3.1.0 for the new prop to work. Although d3-geo@3 depends on d3-array@2.5.0 - 3, this change may be blocked by #1267.

WDYT folks?

@kachkaev kachkaev changed the title Support geoPath digts in <Projection /> component Support geoPath.digits(N) in <Projection /> component Oct 23, 2023
@kachkaev kachkaev changed the title Support geoPath.digits(N) in <Projection /> component Support geoPath.digits(n) in <Projection /> component Oct 23, 2023
@kachkaev kachkaev changed the title Support geoPath.digits(n) in <Projection /> component Support geoPath.digits(n) in Projection components Oct 23, 2023
@kachkaev kachkaev changed the title Support geoPath.digits(n) in Projection components Support geoPath.digits(n) in visx-geo projection components Oct 23, 2023
@kachkaev kachkaev changed the title Support geoPath.digits(n) in visx-geo projection components Support geoPath.digits(n) in geo projection components Oct 23, 2023
@kachkaev
Copy link
Contributor Author

kachkaev commented Oct 27, 2023

@williaster would you be willing to review a PR if I manage to get this task done in a few days? I guess I’ll need to add d3-geo to @visx/vendor but I guess it will be similar to #1678.

@williaster
Copy link
Collaborator

@kachkaev thanks for opening the issue with such great details. I'd be happy to review the change 👀 and get it in if you'd like to take it on 🙏

You are 100% correct about adding @visx/geo to @visx/vendor because it an ESM-only package and therefore would break our dual CJS / ESM exports. We really tried to make this simple for contributors, so hopefully all you need to do to bump the package is to:

lmk if you have any questions along the way – here or on a new PR

@kachkaev
Copy link
Contributor Author

Great, thanks for your reply and for the hints! I’ll try to produce something by the end of this week if things go as planned 🤞

@kachkaev
Copy link
Contributor Author

kachkaev commented Nov 9, 2023

👋 @williaster! I have started a PR in #1767. The description is missing – feel free to help me with it if it’s quick for you. Apart from that, I seem to have produced the diff that works as expected. Happy to do more iterations on this PR, it’d be just great to get some initial feedback from you or your colleagues. The DX was neat, nice setup!

@williaster
Copy link
Collaborator

Amazing! I'll take a look asap today/tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants