Skip to content

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Jan 12, 2024

Issue number: internal


What is the current behavior?

ion-button is being used as a way to open the alerts. It's not being used to test functionality.

When ion-button is updated then the screenshots in the alert tests must be updated. This shouldn't happen when ion-button isn't necessary to have in these tests.

What is the new behavior?

  • Replaced ion-button with button

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@github-actions github-actions bot added the package: core @ionic/core package label Jan 12, 2024
@thetaPC thetaPC marked this pull request as ready for review January 12, 2024 22:42
@thetaPC thetaPC requested a review from a team as a code owner January 12, 2024 22:42
@thetaPC thetaPC requested review from liamdebeasi and removed request for a team January 12, 2024 22:42
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.

Overall looks good! One comment on the expand usage since we don't need it anymore.

Comment on lines 30 to 33
<button expand="block" class="blue" onclick="presentAlert()">Alert</button>
<button expand="block" class="light-blue" onclick="presentAlertLongMessage()">Alert Long Message</button>
<button expand="block" class="red" onclick="presentAlertMultipleButtons()">Multiple Buttons (>2)</button>
<button expand="block" class="light" onclick="presentAlertNoMessage()">Alert No Message</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

expand="block" is an ion-button API, so it should be removed if we are going to use button elements. (Same for all other instances of this)

@thetaPC thetaPC requested a review from liamdebeasi January 15, 2024 17:38
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.

Thank you!

@thetaPC thetaPC added this pull request to the merge queue Jan 15, 2024
Merged via the queue into main with commit 33aa8e3 Jan 15, 2024
@thetaPC thetaPC deleted the alert-ion-button branch January 15, 2024 22:21
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.

3 participants