-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
[d3-shape] Update to Version 1.1 #16508
Conversation
* [Feature] Add stackOffsetDiverging(...) * [Chore] Update version numbers * [Chore] Fix typos in JSDoc comments.
* [Feature] Add definition support for horizontal, vertical and radial link shape generator.
* [Fix] Added `"callable-types": false` to `tslint.json` as the two line-level disablers in `index.d.ts` seemed to be ignored. * [Chore] Fixed some linting errors in text file
types/d3-shape/index.d.ts to authors (@tomwanzek @gustavderdrache @borisyankov). Could you review this PR? Checklist
|
@gustavderdrache when you have a minute, could you kindly have a look over this PR. It's blocking the pipeline a little, as PR #16536 depends on a tslint fix in it. Also, d3-shape 1.2 was just released which should be quick as a follow-up... Just want to avoid disconnecting too much from the actual D3 dependencies. Thanks. |
* | ||
* @param context null, to remove rendering context. | ||
*/ | ||
context(context: null): this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't consider this a blocker for now, but it's something to consider: we should really clean these up to use unions, since you can't preform a round trip:
const context = link.context();
// fails because the overloads aren't compatible with the return value of `.context()`:
link.context(context);
I'd like to see a cleanup so that we don't have to keep disabling unified-signatures
in tslint.json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should create a new tracking issue to discuss some of the clean-up/enhancements we need to consider an create a little roadmap (clean-up of current, TS2.2+ or TS2.3+ feature additions, transition planning)
So the unified signatures should definitely be a discussion item in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create one, when I have a second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@andy-ms Could you kindly merge this PR. Thx in advance. T |
@gustavderdrache This one is admittedly a bit more review effort, but the newly included functionality is all the more worth it...
Closes #16506.
Please fill in this template.
npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
tslint.json
containing{ "extends": "dslint/dt.json" }
.