-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(playwright-html-report): added global checkbox for enabling/disabling html snippets to Playwright Report #35203
base: main
Are you sure you want to change the base?
Conversation
Added functionality such that enabling the checkbox shows snippets, while disabling, hides snippets Co-authored-by: Asher Alacar <[email protected]> Co-authored-by: Jiayi Chen <[email protected]>
…box state Co-authored-by: Agamjot Singh <[email protected]> Co-authored-by: Jiayi Chen <[email protected]>
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"3 flaky38771 passed, 808 skipped Merge workflow run. |
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.
@agamjots05 Could you please include a screenshot that demonstrates the new feature? It is particularly important for UI changes, because it's hard to assess otherwise.
@@ -0,0 +1,21 @@ | |||
/* |
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 copyright seems to omit empty lines for some reason.
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
.setting { |
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.
Could you please format the source consistently with other files, keeping the same indentation?
@@ -0,0 +1,47 @@ | |||
/* |
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.
Same comment here about the copyright being off.
}; | ||
|
||
export const CheckBox: React.FunctionComponent<{ | ||
checkBoxSettings: Check<boolean>[]; |
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.
Why are there multiple "settings" passed to a single checkbox?
import * as React from 'react'; | ||
import './checkbox.css'; | ||
|
||
export type Check<T> = { |
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.
Same comment here, please format consistently.
@@ -71,6 +72,7 @@ export const TestResultView: React.FC<{ | |||
test: TestCase, | |||
result: TestResult, | |||
}> = ({ test, result }) => { | |||
const [showSnippets, setShowSnippets] = React.useState(true); |
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 think this should be a global setting applying to all test results, so it must live somewhere higher up in the components tree.
await thirdTreeItem.click(); | ||
|
||
const codeSnippets = page.locator('[data-testid="test-snippet"]'); | ||
await expect(codeSnippets.first()).toBeVisible(); |
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.
It is better to use toHaveCount(3)
with the right number here, and toHaveCount(0)
to check that snippets are hidden.
fixes #34052
Before:

After:

Not Showing Snippets:
Showing Snippets
