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(menu): apply strict type for menu type #28982

Merged
merged 4 commits into from Mar 9, 2024
Merged

Conversation

sean-perkins
Copy link
Contributor

Issue number: N/A


What is the current behavior?

ion-menu currently accepts any string to the type property, even though only overlay, push and reveal are only supported.

What is the new behavior?

  • Adds more explicit types to the type property on ion-menu

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: core @ionic/core package label Feb 6, 2024
@sean-perkins sean-perkins marked this pull request as ready for review February 9, 2024 01:31
Copy link
Contributor

@amandaejohnston amandaejohnston left a comment

Choose a reason for hiding this comment

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

Should this be added to the breaking changes guide? I'm genuinely not sure; I know that any other strings wouldn't have worked anyway, but technically people could be using other things which would now error.

@sean-perkins
Copy link
Contributor Author

@amandaejohnston I opted to leave it out of the breaking changes, since only misconfigurations should be affected. We aren't removing any options that previously existed. Typescript compiler/type checking should inform the developer clearly of the accepted type.

Still targeted it for v8, since type changes (introducing strictness) are considered breaking.

@sean-perkins sean-perkins merged commit 03d2670 into feature-8.0 Mar 9, 2024
44 checks passed
@sean-perkins sean-perkins deleted the sp/menu-type branch March 9, 2024 20:37
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.

None yet

2 participants