-
Notifications
You must be signed in to change notification settings - Fork 13.4k
test(tab-bar): add snapshot for box shadow #30134
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thetaPC
left a comment
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.
The PR also needs:
- a test test, not feat, since you're adding a test
- not be redundant, the component is already mentioned in the scope so no need to mention it again. The same goes for the type.
test(tab-bar): add snapshot for box shadow
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.
Snapshots should always aim to be as small as possible. That's why the original test was using tabBar instead of content. Since we want to capture the box shadow, then create a container that has padding and use the container for the snapshot instead for both tests. Here's some pseudo code:
.container {
padding: 20px 10px;
}
<ion-content>
<div class="container">
<ion-tab-bar>
</div>
</ion-content>
const container = page.locator('.container');
await expect(container).toHaveScreenshot...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.
Done. The compact snapshots look a bit strange though.
By the way, I ended up not using the container in your suggestion seen as though it had no effect on the snapshots.
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.
This is because I used pseudo code as in my code is only semi complete so:
- Please use the proper syntax
- Add the suggested code to the correct locations
- use the container
.container {
padding: 20px 10px;
}
<ion-content>
<div class="container">
<ion-tab-bar> // not complete, use proper syntax
</div>
</ion-content>
const container = page.locator('.container');
await expect(container).toHaveScreenshot... // not complete, use proper syntaxThere 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 hope I have addressed your concern with my latest commit. The only adjustment I had to do was to manually set the container dimensions on the compact tab bar test since, with only the padding attribute set up, the container shown in the snapshot did not fit the tab bar for some reason and was hiding it instead:

If you have any suggestion for better working around this, please let me know.
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 went ahead and made the appropriate changes since it seems that my comments regarding using proper syntax is not being understood well. I would highly recommend reviewing my changes since it showcases how tests should be written for Ionic. Thanks!
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.
This is because I used pseudo code as in my code is only semi complete so:
- Please use the proper syntax
- Add the suggested code to the correct locations
- use the container
.container {
padding: 20px 10px;
}
<ion-content>
<div class="container">
<ion-tab-bar> // not complete, use proper syntax
</div>
</ion-content>
const container = page.locator('.container');
await expect(container).toHaveScreenshot... // not complete, use proper syntax
thetaPC
left a comment
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.
LGTM, assigning to Brandy for a second check. Once she gives an approval, then this PR can be merged.
| </ion-tab-button> | ||
| </ion-tab-bar> | ||
| `, | ||
| <ion-content> |
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.
We should remove ion-content as this is not needed
brandyscarney
left a comment
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.
Approving with the removal of ion-content 🙂
Issue number: internal
What is the current behavior?
This PR is a follow-up of this one: #30117
What is the new behavior?
Does this introduce a breaking change?
Other information