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

feat(ts): Improve useDialogPluginComponent types #11651

Conversation

MT224244
Copy link
Contributor

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Documentation
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • It's been tested on a Cordova (iOS, Android) app
  • It's been tested on a Electron app
  • Any necessary documentation has been added or updated in the docs (for faster update click on "Suggest an edit on GitHub" at bottom of page) or explained in the PR's description.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

When using useDialogPluginComponent, you can specify the type of the onDialogOK argument.

Also, if you don't specify it, any will be inserted, so it won't be broken.

const { dialogRef, onDialogOK } = useDialogPluginComponent<string>();
onDialogOK("foo"); // OK
onDialogOK(10); // Error

And since ok in emitsObject did not have an argument, I added one.
However, I couldn't think of a way to specify the type here, so it is any...

Copy link
Member

@yusufkandemir yusufkandemir left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Looks good to me 👍

It's not possible to get the payload type to Dialog.create(...).onOk(...) or $q.dialog(...).onOk(...) automatically, at least I am not aware of such possibility in general. But this is still very useful.

onDialogCancel: () => void;
};
emits: ['ok', 'hide'];
emitsObject: {
ok: () => true;
ok: (payload?: any) => true;
Copy link
Member

Choose a reason for hiding this comment

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

Just some insights and thinking:

onDialogOK is basically emitting the given payload, and calls hide to properly hide the dialog. Although the consumers don't really supposed to be emitting this event themselves, we still may consider improving it, so T should have been used instead of any if it was possible.

function onDialogOK (payload) {
emit('ok', payload)
hide()
}

The only way to achieve such a thing would be creating the function form of emitsObject, but it wouldn't really be worth doing it since it will not be used directly anyways. So, any is the best thing possible in the current situation.

@rstoenescu rstoenescu merged commit baa3e79 into quasarframework:dev Dec 13, 2021
@MT224244 MT224244 deleted the improve-useDialogPluginComponent-types branch December 13, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants