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

fix(action-sheet, alert, toast): button roles autocomplete with available options #27940

Merged
merged 7 commits into from
Jan 30, 2024

Conversation

izyuumi
Copy link
Contributor

@izyuumi izyuumi commented Aug 5, 2023

Issue number: resolves #27965


https://www.youtube.com/watch?v=a_m7jxrTlaw&list=PLIvujZeVDLMx040-j1W4WFs1BxuTGdI_b&index=3

What is the current behavior?

There is a lack of autocomplete support for AlertButton attribute of role (there may be similar situations for other components too, however, I was working on this component and found it).

What is the new behavior?

  • Support for autocomplete for the two types defined cancel and destructive, and also supports any arbitrary string if passed in.

Does this introduce a breaking change?

  • Yes
  • No

Other information

I suggest there to be some global interface similar to the following:

interface LooseAutocomplete<T extends string> = T | Omit<string, T>;

I referenced this great video from Matt @mattpocock

@stackblitz
Copy link

stackblitz bot commented Aug 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Aug 5, 2023
amb136

This comment was marked as off-topic.

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Is there an open issue for this problem? If not, can you please create a new issue and link it to this PR?

Thanks!

@@ -45,7 +45,7 @@ type AlertButtonOverlayHandler = boolean | void | { [key: string]: any };

export interface AlertButton {
text: string;
role?: 'cancel' | 'destructive' | string;
role?: 'cancel' | 'destructive' | Omit<string, 'cancel' | 'destructive'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to avoid having to re-type the role in multiple places.

Instead, I would consider something similar to what we did for colors to solve a similar problem when using a type union with string:

https://github.com/ionic-team/ionic-framework/pull/25347/files#diff-83006355bd18aced9f88fd3b27ff6a40527c713b314145c2a34ad54b6361fa87

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a global interface similar to what I mentioned in the Other information section of the initial comment?
If not, I will implement what you have suggested, and continue for now. However, I would suggest there to be something similar, as Ionic is flexible and these interfaces, I believe, are useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

👋 currently the interface.d.ts file acts as the global interface. LiteralUnion is currently an internal type, but we could export it to be used for these changes as well: https://github.com/ionic-team/ionic-framework/blob/main/core/src/interface.d.ts#L132

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I've exported the LiteralUnion type 407ee7c, then updated the unions of ActionSheet, Alert, and Toast 67adfba.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thank you! I'll review and add another member of the team for an additional review as well 👍

@izyuumi
Copy link
Contributor Author

izyuumi commented Aug 10, 2023

Is there an open issue for this problem? If not, can you please create a new issue and link it to this PR?

Thanks!

I've created an issue and linked it to the initial comment I made to this PR (#27965).

@sean-perkins sean-perkins changed the title Add loose autocomplete for AlertButton interface fix(alert): alert button types autocomplete with available options Aug 17, 2023
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

It looks like there are some build errors when compiling the core project:

[ ERROR ]  TypeScript: src/components/toast/toast.tsx:372:18
           Argument of type 'LiteralUnion<"cancel", string> | undefined' is not
           assignable to parameter of type 'string | undefined'.Type
           'Pick<string, never> & { _?: undefined; }' is not assignable to type
           'string | undefined'.

    L371:  const role = button.role;
    L372:  if (isCancel(role)) {
    L373:    return this.dismiss(undefined, role);

[ ERROR ]  TypeScript: src/components/toast/toast.tsx:373:38
           Argument of type 'LiteralUnion<"cancel", string> | undefined' is not
           assignable to parameter of type 'string | undefined'.

    L372:  if (isCancel(role)) {
    L373:    return this.dismiss(undefined, role);
    L374:  }

Let me know if you have questions around it.

@izyuumi
Copy link
Contributor Author

izyuumi commented Aug 23, 2023

My apologies. I was importing LiteralUnion from prettier for some reason. It should be fixed, with passing tests and compiles. Please let me know if the issue still occurs.

@izyuumi
Copy link
Contributor Author

izyuumi commented Sep 15, 2023

Any updates on this?

@izyuumi izyuumi requested a review from a team as a code owner January 24, 2024 17:55
@izyuumi izyuumi requested review from liamdebeasi and removed request for a team January 24, 2024 17:55
@liamdebeasi liamdebeasi changed the title fix(alert): alert button types autocomplete with available options fix(action-sheet, alert, toast): button roles autocomplete with available options Jan 29, 2024
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Tested on my end with dev build 7.6.7-dev.11706625796.1b3c1ab2, and it works great. Thanks for fixing this!

Sample: https://stackblitz.com/edit/8bcriu?file=src%2Fmain.tsx,src%2FApp.tsx

@liamdebeasi liamdebeasi added this pull request to the merge queue Jan 30, 2024
Merged via the queue into ionic-team:main with commit f6fc22b Jan 30, 2024
46 checks passed
@liamdebeasi
Copy link
Contributor

Thank you for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: AlertButton types do not autocomplete with available options
4 participants