-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update Reports for V2 Test Format #845
Conversation
1f67191
to
1f797df
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.
I left a couple of comments but generally looks good. I'm currently working out a way to run this code so stand by for any feedback that comes from when I do that.
return ( | ||
<> | ||
{isReportsPage ? <h3>Results for each command</h3> : null} |
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 this headline is not always needed maybe it's a sign it doesn't belong in this component and can be handled by the parent component.
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.
Good point, addressed in 9eb5875.
{passedAssertions.length} passed, | ||
{failedAssertions.length} failed | ||
</h3> | ||
{isReportsPage ? ( |
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.
Instead of changing the behavior based on an isReportsPage
prop, which is difficult to understand without delving in here, maybe we can add a headingLevel={3}
prop. It would work like this ...
const Header = `h${headingLevel}`
return (
// ...
<Header>
Header content
</Header>
)
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.
Thanks for that, been seeing this sprinkled around! We could make general component for this pattern now. Also updated in 9eb5875.
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.
Able to test locally with no issues. The code looks good to me. Thanks @howard-e !
133ed89
to
8dd8119
Compare
08194df
to
38bec5f
Compare
See #744. Based on #840.
This PR does the following:
The other items seem to already be addressed.