[dagster-azure]: Frontend #26984
[dagster-azure]: Frontend #26984
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 2e8dfb4. |
mlarose
force-pushed
the
mlarose/AD-721-graphql
branch
from
bd3f4cd to
347f9b9
Compare
mlarose
force-pushed
the
mlarose/AD-721-frontend
branch
from
ef80707 to
1b3dd1e
Compare
|
|
||
| const copy = useCopyToClipboard(); | ||
| const onClickFn = async (key: string, value: string | undefined) => { | ||
| if (!value) { |
There was a problem hiding this comment.
the check is necessary because value could be undefined.
There was a problem hiding this comment.
what does it show if the value is undefined?
There was a problem hiding this comment.
actually it's more dealing with the type checker here.
the function cannot be called because the blocks that would make it available would not be rendered
mlarose
force-pushed
the
mlarose/AD-721-frontend
branch
from
1b3dd1e to
2e8dfb4
Compare
| try { | ||
| const metadata = JSON.parse(logCaptureInfo.logManagerMetadata); | ||
| switch (metadata.type) { | ||
| case 'AzureBlobComputeLogManager': | ||
| if (metadata.storage_account && metadata.container) { | ||
| return `az storage blob download --account-name ${metadata.storage_account} --container-name ${metadata.container} --name ${path}`; | ||
| } | ||
| } | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
| return undefined; | ||
| }; |
There was a problem hiding this comment.
This generally lgtm; my only question; is the shell command copyable by clicking? It would be nice to enable that if it's not hard.
Will leave final review to @hellendag
| <Box flex={{direction: 'row', alignItems: 'center', gap: 8}}> | ||
| <a href={externalUrl} target="_blank" rel="noreferrer"> | ||
| {externalUrl} | ||
| <Icon name="open_in_new" style={{display: 'inline-block'}} /> | ||
| </a> | ||
| </Box> |
There was a problem hiding this comment.
I suggest putting the Box inside the a, since the icon is currently a bit misaligned with the text.
<a href="...">
<Box ...>
<span>{externalUrl}</span>
<Icon ... />
</Box>
</a>
There was a problem hiding this comment.
thanks for the review, should I keep the Box outside of it? ie:
<td>
<Box>
<a>
<Box>
<span />
<icon />
</Box>
</a>
</Box>
<td>There was a problem hiding this comment.
I don't think you should need to. Rendering in table cells can do some unexpected things, but if it looks fine without the wrapper then no need to include one.
|
Yes, it's copiable by clicking. It's implemented with the |
## Summary & Motivation Added support for displaying shell commands in the captured logs panel. When logs are captured by supporting ComputeLogManagers (Azure for now), users can now view and copy these commands directly from the UI, making it easier to obtain logs. ## Notes to reviewers Rework of #26984 with following changes: - graphql data structure changed - shell cmd is generated earlier - removed the uri / path (not useful; less is more) - stylng fixes (added the recommended Box) ## How I Tested These Changes - See top stack PR
Summary & Motivation
How I Tested These Changes
Reviewer's note
Is the styling (code + result) adequate?
Screenshot