Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
I have added some questions / comments but I am approving this as you can tell me if you disagree on any or all.
| import { configs, test } from '@utils/test/playwright'; | ||
|
|
||
| /** | ||
| * Color doesn't change with direction, so this only runs ltr. |
There was a problem hiding this comment.
Is this the new way we are going to write these comments? Typically we write the following:
| * Color doesn't change with direction, so this only runs ltr. | |
| * This behavior does not vary across directions. |
| */ | ||
| configs({ modes: ['ionic-md', 'md', 'ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('title: color'), () => { | ||
| test('should apply the color to the title text', async ({ page }) => { |
There was a problem hiding this comment.
Can we standardize on how we title tests? This is the usual title:
| test('should apply the color to the title text', async ({ page }) => { | |
| test('should not have visual regressions', async ({ page }) => { |
| configs({ modes: ['ionic-md', 'md', 'ios'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('title: basic'), () => { | ||
| test('should not have visual regressions', async ({ page }) => { | ||
| await page.goto('/src/components/title/test/basic', config); |
There was a problem hiding this comment.
We have been leaning toward migrating tests to page.setContent, is there a reason we aren't doing this here?
There was a problem hiding this comment.
Do we need an index.html preview for the color? I am fine if the answer is yes but we will probably need to add it for other components.
| configs({ modes: ['ionic-md', 'md', 'ios'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | ||
| test.describe(title('title: color'), () => { | ||
| test('should apply the color to the title text', async ({ page }) => { | ||
| await page.goto('/src/components/title/test/color', config); |
There was a problem hiding this comment.
We have been leaning toward migrating tests to page.setContent, is there a reason we aren't doing this here?
See Breadcrumbs
I don't think we need to capture each color for each component. If we have proper screenshot tests in the theme folder capturing the colors then we should just need to do a few colors in the components themselves. Let me know your thoughts.
| }); | ||
| }); | ||
|
|
||
| describe('title: style event', () => { |
There was a problem hiding this comment.
Could we also cover this event being emit on the first connect?
This branch was successfully deployed
2 participants
Issue number: resolves internal
What is the current behavior?
No test sets
coloronion-title, and no test screenshots a title in the ionic theme in RTL.What is the new behavior?
colortest page and screenshots in the ionic, md and ios themes.basicscreenshots in RTL too. The ios and md RTL snapshots already existed without a test generating them; they now have one..toolbar-titleelement the iOS large title animations measure, and theionStyleevent.Does this introduce a breaking change?
Other information