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

Fix center and add pointRadius, innerRef #213

Merged
merged 3 commits into from
Dec 11, 2017
Merged

Conversation

msathis
Copy link

@msathis msathis commented Dec 11, 2017

💥 Breaking Changes

None.

🚀 Enhancements

Added pointRadius and fixed center.

📝 Documentation

🐛 Bug Fix

Fixed center typo.

🏠 Internal

@hshoff hshoff added this to the v0.0.150 milestone Dec 11, 2017
@hshoff
Copy link
Member

hshoff commented Dec 11, 2017

LGTM thank you!

@hshoff hshoff merged commit 73cdf53 into airbnb:master Dec 11, 2017
if (rotate) currProjection.rotate(rotate);
if (precision) currProjection.rotate(precision);
if (fitExtent) currProjection.fitExtent(...fitExtent);
if (fitSize) currProjection.fitSize(...fitSize);

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

if (pointRadius) path.pointRadius(pointRadius);
Copy link
Contributor

@kachkaev kachkaev Nov 9, 2023

Choose a reason for hiding this comment

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

Suggested change
if (pointRadius) path.pointRadius(pointRadius);
if (pointRadius !== undefined) path.pointRadius(pointRadius);

👋 folks! Do you if pointRaidus can be 0? Would it be a valid input? I can change this in #1767 if it makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @kachkaev according to the d3-geo docs this should be a 2-element numerical array https://d3js.org/d3-geo/projection#projection_center

that's consistent with our @visx/geo types / docs https://airbnb.io/visx/docs/geo#Projection_center

so I don't think 0 is valid.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it’s related to https://d3js.org/d3-geo/path#path_pointRadius (one number)

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤦 🤦 🤦 sorry no idea how I misread that one. 100% agree with you/thanks for adding in your new PR 🙏

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.

4 participants