-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add log link next to status message #367
Add log link next to status message #367
Conversation
Do the docs need to be updated for a change this small? |
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 self-reviewing
</Typography> | ||
</StyledMetadataItem> | ||
</> | ||
)} | ||
{!currentBuild && ( | ||
) : ( |
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.
Out of curiosity, how come this is needed? Are there cases where currentBuild
is undefined? Looking through buildMapper
it seems like the currentBuildId
should always have a corresponding item in builds
.
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.
Yeah... that's a good question. I don't know... Pagination maybe?
It doesn't help that the variable currentBuild
does not actually correspond the currentBuildId
build, see line 28 above:
const currentBuild = envBuilds.find(build => build.id === selectedBuildId);
So maybe there's some way you can select a build id that isn't in the builds for that environment?
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 don't have the context to know whether an additional refactor here is appropriate, but you might. If you feel like this could be cleaner, please make an issue for it. In the meantime, let's get this merged.
2f71e21
to
95bdda8
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.
Thanks @peytondmurray!
</Typography> | ||
</StyledMetadataItem> | ||
</> | ||
)} | ||
{!currentBuild && ( | ||
) : ( |
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.
Yeah... that's a good question. I don't know... Pagination maybe?
It doesn't help that the variable currentBuild
does not actually correspond the currentBuildId
build, see line 28 above:
const currentBuild = envBuilds.find(build => build.id === selectedBuildId);
So maybe there's some way you can select a build id that isn't in the builds for that environment?
I do not know why the Playwright test is failing for this PR |
95bdda8
to
4517804
Compare
OK, test passing now, this just needs to be approved |
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.
Approved, see note though about filing an issue if a refactor of EnvBuilds.tsx
is appropriate. The fact that it's confusing for both of us probably means we should track an issue even if we don't work on it right away, but I'll defer to your judgment about this.
Fixes #244. (But note that in the interest of time, I did not follow the design mocks exactly.)
Description
This pull request:
<EnvBuilds>
component if the currently selected build status is "Failed"Pull request checklist