Skip to content

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Nov 6, 2023

Issue number: N/A


What is the current behavior?

As a takeaway from our learning session about a menuController bug in Ionic Angular, the team would like to update our other providers to use the same architecture as the menuController to prevent this kind of issue from happening again in the future.

We also noticed that the common provider does not provide much value and it's easier to just have two separate implementations in src and standalone. (There wasn't much code we could de-duplicate)

What is the new behavior?

  • Removed the common action sheet provider in favor of separate ones in src/standalone

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added the package: angular @ionic/angular package label Nov 6, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review November 6, 2023 21:20
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@liamdebeasi liamdebeasi changed the title refactor(angular): provider imports correct instance from core refactor(angular): action sheet provider imports correct instance from core Nov 7, 2023
@liamdebeasi liamdebeasi requested a review from thetaPC November 7, 2023 19:17
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@liamdebeasi liamdebeasi added this pull request to the merge queue Nov 8, 2023
Merged via the queue into main with commit c5dd622 Nov 8, 2023
@liamdebeasi liamdebeasi deleted the FW-5476 branch November 8, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: angular @ionic/angular package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants