-
Notifications
You must be signed in to change notification settings - Fork 86
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
Port embed test to playwright #4440
Port embed test to playwright #4440
Conversation
…nge request handling inside EmbedInput
page.locator(s('authoring', 'authoring-field=body_html')).getByRole('textbox'), | ||
).toHaveText('test sport story body'); | ||
|
||
await page.getByRole('button', {name: 'Embed'}).click(); |
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.
use a more specific selector
|
||
await page.getByRole('button', {name: 'Embed'}).click(); | ||
|
||
await page.getByPlaceholder('Enter URL or code to embed').type('https://sourcefabric.org'); |
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.
use a more specific selector
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.
Let's agree that comments are only "resolved" on github by the person reviewing it. I did unresolve a few that weren't addressed.
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.
do handle conflicts
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.
if you remove data-test-id
, remove data-test-value
as well
@tomaskikutis can you resolve all comments that are addressed in the code |
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.
All good now, let's get tests passing before merging. Unit tests you need to look at and e2e tests I'll take a look at because I saw they fail on develop.
No description provided.