[4/n] Add UI changes to render deleted artifacts #1401
Conversation
| let iconColor = theme.palette.black; | ||
| let checkIcon = faMinus; | ||
| if (value) { | ||
| return <>{value}</>; |
There was a problem hiding this comment.
Does this need to be a Typography component? Or does the parent component take care of this?
There was a problem hiding this comment.
This follows what has been used in main version that we returns the raw string for check names, etc.
There was a problem hiding this comment.
We could use a Typography here but I feel that's an independent change and not necessarily need to spend much time in this PR
There was a problem hiding this comment.
no worries, just checking what's going on here.
| monochrome={false} | ||
| /> | ||
| ); | ||
| if (status) { |
There was a problem hiding this comment.
This can be combined with the return statement below as a ternary to check for status.
status={status ? status : ExecutionStatus.Unknown}
| @@ -45,7 +45,7 @@ const ArtifactContent: React.FC<Props> = ({ | |||
| ); | |||
| } | |||
|
|
|||
| if (!content || !artifactResult) { | |||
| if (!content || !content.content || !artifactResult) { | |||
There was a problem hiding this comment.
Could this become !content?.content || !artifactResult ?
…to eng-3000-allow-users-to-opt-out-of-data-snapshots-4
…to eng-3000-allow-users-to-opt-out-of-data-snapshots-4
|
Based on discussion with @vsreekanti , we make the delete artifact green but showing a different icon / description to distinguish from the succeeded one. I deliberately make the icon gray so that the node is not 'as green as' a succeeded one |
|
Nice work! Should we gray out / add some sort of opacity filter over the side sheet so that users know that the artifact has been deleted. I find it a little hard to distinguish that this artifact is no longer there. |
|
Maybe something that we can also do is add a strikethrough to the text of the artifact name in the DAG. Open to ideas here @vsreekanti |
likawind
merged commit 552bddb
into
eng-3000-allow-users-to-opt-out-of-data-snapshots-3
Describe your changes and why you are making these changes
This PR adds necessary changes to render artifacts who has snapshots disabled. Note that we show the results that get disabled but not implying that future workflow run will have the snapshot disabled.
We also added some small fix so that we show check status that aligns with op results rather than artf results
Related issue number (if any)
ENG-3000
Loom demo (if any)
Verified in manual QA that no regressions to relevant features:
https://www.loom.com/share/12a3015af65b47e6b2511c17aeab1c6b
Verified on the manual QA with disabled API calls:
Checklist before requesting a review
python3 scripts/run_linters.py -hfor usage).run_integration_test: Runs integration testsskip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)