Skip to content

CropperJS - Use partials for setting methods which take options#24959

Merged
RyanCavanaugh merged 1 commit intoDefinitelyTyped:masterfrom
happycollision:fix-cropperjs-set-types
Apr 18, 2018
Merged

CropperJS - Use partials for setting methods which take options#24959
RyanCavanaugh merged 1 commit intoDefinitelyTyped:masterfrom
happycollision:fix-cropperjs-set-types

Conversation

@happycollision
Copy link
Copy Markdown
Contributor

Please fill in this template.

  • 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.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

This commit addresses a problem where the following would produce a type
error, even though it was valid code:

setCanvasData({left: 23})

I have changed the argument type for all of the setXXX methods which
accept an object as their argument. In the spec files for cropperjs,
@fengyuanchen uses these objects as partials for setters. So I have
made the type definitions follow that same pattern.

I changed the cropperjs.CropBoxData type to act the same way as well.
It looks like it was changed at some point, I am guessing to allow it
to be used as a partial in the setCropBoxData method. Since, like all
the other data structures here, it contains all members when you get
it, I have made all members required parts of the object and changed
the setter method to accept a partial.

I hope this all makes sense. Basically, I went from the first line below
to the second.

setXXXXX(thing: cropperjs.SomeInterface): void

setXXXXX(thing: Partial<cropperjs.SomeInterface>): void

Then I changed cropperjs.CropBoxData to fit this new system.

This commit addresses a problem where the following would produce a type
error, even though it was valid code:

```typescript
setCanvasData({left: 23})
```

I have changed the argument type for all of the `setXXX` methods which
accept an object as their argument. In the spec files for cropperjs,
@fengyuanchen uses these objects as partials for setters. So I have
made the type definitions follow that same pattern.

I changed the `cropperjs.CropBoxData` type to act the same way as well.
It looks like  it was changed at some point, I am guessing to allow it
to be used as a partial in the `setCropBoxData` method. Since, like all
the other data structures here, it contains all members when you `get`
it, I have made all members required parts of the object and changed
the setter method to accept a partial.

I hope this all makes sense. Basically, I went from the first line below
to the second.

```typescript
setXXXXX(thing: cropperjs.SomeInterface): void

setXXXXX(thing: Partial<cropperjs.SomeInterface>): void
```

Then I changed `cropperjs.CropBoxData` to fit this new system.
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 12, 2018

@happycollision Thank you for submitting this PR!

🔔 @stepancar - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Apr 17, 2018
@typescript-bot
Copy link
Copy Markdown
Contributor

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@RyanCavanaugh RyanCavanaugh merged commit 932727f into DefinitelyTyped:master Apr 18, 2018
@RyanCavanaugh
Copy link
Copy Markdown
Member

🌟 🎈 🎉 🏆 🎂 ✨ ⭐️

Congratulations on your first DefinitelyTyped contribution!

🌟 🎈 🎉 🏆 🎂 ✨ ⭐️

@happycollision happycollision deleted the fix-cropperjs-set-types branch April 18, 2018 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Unmerged The author did not merge the PR when it was ready.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants