Wizard: Update package step (HMS-10579)#4494
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #4494 +/- ##
=======================================
Coverage 75.34% 75.34%
=======================================
Files 229 229
Lines 7446 7447 +1
Branches 2768 2771 +3
=======================================
+ Hits 5610 5611 +1
Misses 1578 1578
Partials 258 258
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This: - adds a placeholder to the package search - adds a required label as per revamp SPUR.
d39cc4e to
5a5712f
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
placeholder='Add packages'is hard-coded and not aligned with the dynamicpackageTypeLabelused in thearia-label; consider deriving the placeholder frompackageTypeLabel(or making it a prop) so it stays consistent and easier to adjust per package type. - For the
Requiredlabel next topkg.name, consider whether this should have a specific variant (e.g.variant='outline'orcolorif supported) or a dedicated class for spacing/visual distinction so it doesn’t visually merge with longer package names.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `placeholder='Add packages'` is hard-coded and not aligned with the dynamic `packageTypeLabel` used in the `aria-label`; consider deriving the placeholder from `packageTypeLabel` (or making it a prop) so it stays consistent and easier to adjust per package type.
- For the `Required` label next to `pkg.name`, consider whether this should have a specific variant (e.g. `variant='outline'` or `color` if supported) or a dedicated class for spacing/visual distinction so it doesn’t visually merge with longer package names.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/Packages/components/PackagesTable.tsx" line_range="242-243" />
<code_context>
>
<Td> </Td>
- <Td>{pkg.name}</Td>
+ <Td>
+ {pkg.name} {isRequired && <Label isCompact>Required</Label>}
+ </Td>
<Td>{pkg.stream ? pkg.stream : '--'}</Td>
</code_context>
<issue_to_address>
**suggestion:** Consider how the inline `Required` label affects localization and layout consistency.
This introduces a new visible string and changes the row layout. If the wizard is localized, this label should use the existing translation mechanism. Also consider how multiple `Required` labels in a column affect density; a shorter token, icon with accessible text, or a standardized PatternFly label variant (e.g., `variant="outline"`) may keep the layout more compact and consistent.
Suggested implementation:
```typescript
import { Content, Label } from '@patternfly/react-core';
import { useIntl } from 'react-intl';
```
```typescript
<Td>
{pkg.name}{' '}
{isRequired && (
<Label
isCompact
variant="outline"
data-testid="required-package-label"
>
{intl.formatMessage({
id: 'wizard.packages.required',
defaultMessage: 'Required',
})}
</Label>
)}
</Td>
```
1. Ensure that this component (or a parent in the same file) initializes `intl` via `const intl = useIntl();`. If it's not present yet, add it at the top level of the functional component body.
2. If your project uses a specific message catalog or ID convention, adjust the `id: 'wizard.packages.required'` to match your existing i18n keys.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <Td> | ||
| {pkg.name} {isRequired && <Label isCompact>Required</Label>} |
There was a problem hiding this comment.
suggestion: Consider how the inline Required label affects localization and layout consistency.
This introduces a new visible string and changes the row layout. If the wizard is localized, this label should use the existing translation mechanism. Also consider how multiple Required labels in a column affect density; a shorter token, icon with accessible text, or a standardized PatternFly label variant (e.g., variant="outline") may keep the layout more compact and consistent.
Suggested implementation:
import { Content, Label } from '@patternfly/react-core';
import { useIntl } from 'react-intl'; <Td>
{pkg.name}{' '}
{isRequired && (
<Label
isCompact
variant="outline"
data-testid="required-package-label"
>
{intl.formatMessage({
id: 'wizard.packages.required',
defaultMessage: 'Required',
})}
</Label>
)}
</Td>- Ensure that this component (or a parent in the same file) initializes
intlviaconst intl = useIntl();. If it's not present yet, add it at the top level of the functional component body. - If your project uses a specific message catalog or ID convention, adjust the
id: 'wizard.packages.required'to match your existing i18n keys.
This:
as per revamp SPUR.