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

Removed incorrect optional labels from tray.displayBalloon(options) docs #10936

Merged
merged 1 commit into from Nov 3, 2017

Conversation

Projects
None yet
4 participants
@robinwassen
Contributor

robinwassen commented Oct 26, 2017

Title and content was marked as optional, but the API throw error if they are not defined.

See: https://github.com/electron/electron/blob/master/atom/browser/api/atom_api_tray.cc#L185

@robinwassen robinwassen requested a review from electron/docs as a code owner Oct 26, 2017

@robinwassen

This comment has been minimized.

Show comment
Hide comment
@robinwassen

robinwassen Oct 26, 2017

Contributor

I have tried to figure out the reason why the typescript definition generator complains about:

test/main.ts(884,26): error TS2345: Argument of type '{ title: string; }' is not assignable to parameter of type 'DisplayBalloonOptions'.
Property 'content' is missing in type '{ title: string; }'.

Can't find any reference to DisplayBalloonOptions other than the place I changed.

Contributor

robinwassen commented Oct 26, 2017

I have tried to figure out the reason why the typescript definition generator complains about:

test/main.ts(884,26): error TS2345: Argument of type '{ title: string; }' is not assignable to parameter of type 'DisplayBalloonOptions'.
Property 'content' is missing in type '{ title: string; }'.

Can't find any reference to DisplayBalloonOptions other than the place I changed.

@ckerr

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Oct 27, 2017

Member

Looks like electron-docs-linter's parser requires a description for each object parameter.

Instead of removing everything after String, try replacing (optional) with a simple description. The parameter name is enough for humans in this case, but the parser needs more help 😐

Member

ckerr commented Oct 27, 2017

Looks like electron-docs-linter's parser requires a description for each object parameter.

Instead of removing everything after String, try replacing (optional) with a simple description. The parameter name is enough for humans in this case, but the parser needs more help 😐

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 27, 2017

Member

@ckerr The descriptions themselves are optional, the issue here is electron-typescript-definitions has tests to ensure the typings work OK and in these tests we have a call to displayBalloon.

See here: https://github.com/electron/electron-typescript-definitions/blob/master/test-smoke/electron/test/main.ts#L884

Basically if we are sure these parameters aren't optional (from the code it looks like only icon is optional, then that test should be updates 👍

Member

MarshallOfSound commented Oct 27, 2017

@ckerr The descriptions themselves are optional, the issue here is electron-typescript-definitions has tests to ensure the typings work OK and in these tests we have a call to displayBalloon.

See here: https://github.com/electron/electron-typescript-definitions/blob/master/test-smoke/electron/test/main.ts#L884

Basically if we are sure these parameters aren't optional (from the code it looks like only icon is optional, then that test should be updates 👍

@robinwassen

This comment has been minimized.

Show comment
Hide comment
@robinwassen

robinwassen Oct 28, 2017

Contributor

@MarshallOfSound Thank you! I opened a PR in that in the TS def repo too.

That it is in a separate repo explains why I could not find it 👍

Contributor

robinwassen commented Oct 28, 2017

@MarshallOfSound Thank you! I opened a PR in that in the TS def repo too.

That it is in a separate repo explains why I could not find it 👍

@robinwassen

This comment has been minimized.

Show comment
Hide comment
@robinwassen

robinwassen Nov 1, 2017

Contributor

@MarshallOfSound My change to the typescript definitions has been merged now. Any chance that you can re-run the Travis build?

Contributor

robinwassen commented Nov 1, 2017

@MarshallOfSound My change to the typescript definitions has been merged now. Any chance that you can re-run the Travis build?

@zcbenz

zcbenz approved these changes Nov 3, 2017

I have triggered the test and it is passing now.

@zcbenz zcbenz merged commit 42d6fe2 into electron:master Nov 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment