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 d3-geo and d3 (#14756) #14831

Merged
merged 4 commits into from
Mar 7, 2017
Merged

Update d3-geo and d3 (#14756) #14831

merged 4 commits into from
Mar 7, 2017

Conversation

Ledragon
Copy link
Contributor

Regarding Issue #14756, this PR includes definition for the new path.measure method in d3-geo and the subsequent versions changes in d3 and d3-geo.

  • Make your PR against the master branch.
  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc without errors.
  • Run npm run lint package-name if a tslint.json is present. no tslint
  • Provide a URL to documentation or source code which provides context for the suggested changes: d3-geo issue 84
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "../tslint.json" }.

cc @tomwanzek @gustavderdrache

@dt-bot
Copy link
Member

dt-bot commented Feb 23, 2017

d3-geo/index.d.ts

to authors (@Ledragon @tomwanzek @gustavderdrache @borisyankov). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

d3/index.d.ts

to authors (@tomwanzek @gustavderdrache @borisyankov). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@tomwanzek
Copy link
Contributor

@Ledragon Thx will go over it before the end of the weekend! Take care, T

Copy link
Contributor

@tomwanzek tomwanzek left a comment

Choose a reason for hiding this comment

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

Thx for the PR, looks great I just added a couple of suggestions for the JSDoc comments (incl. fixes to an error of mine, I had not seen before.)
Otherwise, LGTM

*
* This method observes any clipping performed by the projection.
*
* @param object GeoJSON object to measure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor detail: For the other methods on GeoPath we started the @param description with An object..., We should keep it consistent, as the DatumObject does not have to be equivalent to the narrow definition of GeoJSON object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually changed the whole parameter comment for it to match the sentence structure of the other ones.

@@ -936,6 +936,16 @@ export interface GeoPath<This, DatumObject extends GeoPermissibleObjects> {
centroid(object: DatumObject): [number, number];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please insert object after @param to read @param object An Object.... I must have overlooked the omission, when adding the JSDoc comments originally.

The same omission is also there for the area and bounds method. Thx.
Since we already have a PR open... 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, done now :-)

@tomwanzek
Copy link
Contributor

👍
Thx. I will do d3-geo 1.6 as soon as this PR is merged/published.

@Ledragon
Copy link
Contributor Author

Ledragon commented Mar 5, 2017

Thx to you. Let's hop this gets merged soon then

@tomwanzek
Copy link
Contributor

Could this PR get merged, please?

@minestarks minestarks merged commit 94aa028 into DefinitelyTyped:master Mar 7, 2017
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

4 participants