Wizard: Replace typeahead profile selector with simple dropdown (HMS-10638)#4435
Open
mgold1234 wants to merge 1 commit into
Open
Wizard: Replace typeahead profile selector with simple dropdown (HMS-10638)#4435mgold1234 wants to merge 1 commit into
mgold1234 wants to merge 1 commit into
Conversation
4526197 to
a9e129d
Compare
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #4435 +/- ##
==========================================
+ Coverage 75.34% 75.40% +0.06%
==========================================
Files 229 229
Lines 7446 7409 -37
Branches 2770 2755 -15
==========================================
- Hits 5610 5587 -23
+ Misses 1578 1567 -11
+ Partials 258 255 -3
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
a489a7a to
a44d907
Compare
b62a901 to
f2c7fe4
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
PolicySelector, removing the explicitNoneoption means there’s no way to clear a selected policy while staying incompliancemode; consider retaining a clear/"None" affordance so users don’t have to switch back to "No additional policy or profile" just to unset a policy. - For
ProfileSelector, you now rely solely on the surrounding radios to clear the profile; if users are expected to switch between different OpenSCAP profiles frequently, it may be worth adding an in-control clear mechanism (similar to the policy selector) to avoid forcing a round-trip through the "None" radio state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `PolicySelector`, removing the explicit `None` option means there’s no way to clear a selected policy while staying in `compliance` mode; consider retaining a clear/"None" affordance so users don’t have to switch back to "No additional policy or profile" just to unset a policy.
- For `ProfileSelector`, you now rely solely on the surrounding radios to clear the profile; if users are expected to switch between different OpenSCAP profiles frequently, it may be worth adding an in-control clear mechanism (similar to the policy selector) to avoid forcing a round-trip through the "None" radio state.
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/Oscap/components/ProfileSelector.tsx" line_range="134-138" />
<code_context>
- }
- }, [filterValue, profileDetails, isOpen]);
-
const handleToggle = () => {
- if (!isOpen && complianceType === 'openscap') {
+ if (!isOpen) {
refetch();
}
setIsOpen(!isOpen);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid double-toggling `isOpen` between `MenuToggle` and `Select`’s `onOpenChange`.
`MenuToggle` currently calls `setIsOpen(!isOpen)` on click, and `Select`’s `onOpenChange` calls `handleToggle`, which also flips `isOpen` and triggers `refetch()` when opening. If both fire, the state can flip twice and `refetch` timing becomes unclear. Consider centralizing the toggle and `refetch` in one handler (e.g., have the toggle call `handleToggle` and let `onOpenChange` only sync state, or the reverse) so there’s a single source of truth for open/close behavior.
Suggested implementation:
```typescript
const handleToggle = (nextIsOpen: boolean) => {
if (nextIsOpen && !isOpen) {
refetch();
}
setIsOpen(nextIsOpen);
handleServices(undefined);
dispatch(clearKernelAppend());
dispatch(changeFips(false));
```
To fully avoid the double-toggle issue and centralize the behavior:
1. Update the `MenuToggle` usage in this file from something like:
- `onClick={() => setIsOpen(!isOpen)}`
to:
- `onClick={() => handleToggle(!isOpen)}`
2. Update the `Select` component’s `onOpenChange` from:
- `onOpenChange={handleToggle}` (if currently relying on it to flip state),
to:
- `onOpenChange={(_event, isOpen) => handleToggle(isOpen)}`
so `onOpenChange` simply forwards the new open state to `handleToggle` instead of toggling it again.
These adjustments ensure `handleToggle` is the single source of truth for open/close state and `refetch()` execution, and both `MenuToggle` and `Select` only delegate to it without independently flipping `isOpen`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
dd92752 to
25c087f
Compare
2d68c6b to
a3571d9
Compare
The ProfileSelector used a typeahead MenuToggle variant with text filtering. PatternFly's typeahead variant renders an <input> element whose placeholder color is controlled by a CSS variable that does not inherit the dark mode theme, causing the placeholder text to appear white-on-white. Replace the typeahead with a simple button-based dropdown matching the PolicySelector pattern. This sidesteps the dark mode bug entirely by removing the <input> element. Also adds an applying state with a spinner, and sets isPlaceholder on the PolicySelector toggle for consistency.
a3571d9 to
8efd0c3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The ProfileSelector used a typeahead variant with text filtering,
which caused the placeholder text to appear white in dark mode.
PatternFly's MenuToggle typeahead variant uses a separate CSS variable
for its input placeholder that doesn't inherit the dark mode color.
Replace with a simple button-based dropdown matching the PolicySelector
pattern. This also adds an applying state with a spinner, and adds
isPlaceholder to the PolicySelector toggle for consistency.
JIRA: HMS-10638 HMS-10657