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
test(many): replace ionic buttons in e2e tests with native html buttons #29422
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -143,7 +152,7 @@ <h3>mode: md</h3> | |||
dynamicHomeIcon.name = 'home'; | |||
dynamicHomeIcon.isActive = false; | |||
|
|||
icons = ['home', 'star', 'ios-alert', 'ios-alert-outline', 'md-alert', 'logo-apple']; | |||
icons = ['home', 'star', 'alert-circle', 'alert-circle-outline', 'alert-circle-sharp', 'logo-apple']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this to use the latest Ionicon names by comparing the older names with the v3 Ionicons to see what it should be now: https://ionicframework.com/docs/v3/ionicons
core/scripts/testing/styles.css
Outdated
|
||
button { | ||
display: block; | ||
width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to default to 100% width here because it seemed the more common use case, but I could be convinced to make an inline, auto-width button the default if we prefer that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with the team during office hours. We decided to update the button to be inline-block
by default and then add a class called expand
which replaces the old expand="block"
behavior that using an ion-button
had.
e84370a
to
92792d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question that applies to a few places around the color
attribute on the button
element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Co-authored-by: ionitron <[email protected]>
Issue number: internal
What is the current behavior?
The
ion-button
component is used in several tests to navigate or show overlays. This causes screenshot diffs in unrelated tests any time the UI of theion-button
is updated.What is the new behavior?
Removes the
ion-button
elements from unrelated tests.Did not remove the
ion-button
s from the following tests:ion-button
s in anion-buttons
componention-button
inside of a menuUpdates the icon/basic test to use the right icon names by comparing against the v3 names: https://ionicframework.com/docs/v3/ionicons/
Does this introduce a breaking change?