ROX-33571: Improve container display components#19511
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
ContainerVolumesInfo,String(volume.readOnly)will render"undefined"whenreadOnlyis not set, whereas the previous implementation defaulted to'false'; consider preserving the explicit fallback or normalizing undefined/null to'false'to avoid odd UI output. - In
ContainerArgumentsInfo, you disablereact/no-array-index-keyand use a composite ofargandindexfor the key; ifargsare guaranteed to be unique, usingargalone as the key would avoid the lint override and produce more stable keys.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ContainerVolumesInfo`, `String(volume.readOnly)` will render `"undefined"` when `readOnly` is not set, whereas the previous implementation defaulted to `'false'`; consider preserving the explicit fallback or normalizing undefined/null to `'false'` to avoid odd UI output.
- In `ContainerArgumentsInfo`, you disable `react/no-array-index-key` and use a composite of `arg` and `index` for the key; if `args` are guaranteed to be unique, using `arg` alone as the key would avoid the lint override and produce more stable keys.
## Individual Comments
### Comment 1
<location path="ui/apps/platform/src/Components/ContainerVolumesInfo.tsx" line_range="54-60" />
<code_context>
- <DescriptionList isCompact className="pf-v6-u-p-md">
- <DescriptionListGroup>
- <DescriptionListTerm>Source</DescriptionListTerm>
- <DescriptionListDescription>
- {secret.path}
- </DescriptionListDescription>
</code_context>
<issue_to_address>
**issue (bug_risk):** Rendering `readOnly` directly as `String(volume.readOnly)` may surface `"undefined"` for missing values.
Previously, falsy values defaulted to `'false'`, so absent `readOnly` didn’t render as `
</issue_to_address>
### Comment 2
<location path="ui/apps/platform/src/Components/ContainerArgumentsInfo.tsx" line_range="18-19" />
<code_context>
<Card>
<CardTitle>Arguments</CardTitle>
{args.length > 0 ? (
- <CardBody className="pf-v6-u-pt-lg pf-v6-u-mx-lg pf-v6-u-mb-lg">
- <List isPlain className="pf-v6-u-max-height" style={styleConstant}>
- {args.map((arg) => (
- <ListItem>--{arg}</ListItem>
</code_context>
<issue_to_address>
**issue:** Removing the scroll and max-height behavior may let long argument lists grow unbounded.
With the new plain `<CardBody>` and `<List>`, a large number of arguments could make this card (or the whole page) excessively tall. If that’s not intended, it may be worth reintroducing a max-height with overflow or another layout constraint.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <CardBody className="pf-v6-u-pt-lg pf-v6-u-mx-lg pf-v6-u-mb-lg"> | ||
| <List isPlain className="pf-v6-u-max-height" style={styleConstant}> |
There was a problem hiding this comment.
issue: Removing the scroll and max-height behavior may let long argument lists grow unbounded.
With the new plain <CardBody> and <List>, a large number of arguments could make this card (or the whole page) excessively tall. If that’s not intended, it may be worth reintroducing a max-height with overflow or another layout constraint.
|
Images are ready for the commit at 6592f45. To use with deploy scripts, first |
b9617b8 to
e1376fb
Compare
…g, fix readOnly display
e1376fb to
6592f45
Compare
Description
Refactors container display components to improve readability and fix a display bug:
This prepares the components for consolidation in subsequent PRs.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Part of ROX-33571 container display consolidation stack.