Skip to content

Wizard: Replace typeahead profile selector with simple dropdown (HMS-10638)#4435

Open
mgold1234 wants to merge 1 commit into
osbuild:mainfrom
mgold1234:fix-profile-placeholder-dark-mode
Open

Wizard: Replace typeahead profile selector with simple dropdown (HMS-10638)#4435
mgold1234 wants to merge 1 commit into
osbuild:mainfrom
mgold1234:fix-profile-placeholder-dark-mode

Conversation

@mgold1234
Copy link
Copy Markdown
Collaborator

@mgold1234 mgold1234 commented May 11, 2026

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.

Screenshot 2026-05-20 at 14 40 34

JIRA: HMS-10638 HMS-10657

@mgold1234 mgold1234 changed the title Wizard: use placeholder styling for profile selector in dark mode Wizard: use placeholder styling for profile selector in dark mode (HMS-10638) May 11, 2026
@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch 2 times, most recently from 4526197 to a9e129d Compare May 12, 2026 08:10
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.40%. Comparing base (29382df) to head (8efd0c3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...eWizard/steps/Oscap/components/ProfileSelector.tsx 95.23% 1 Missing ⚠️

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
...geWizard/steps/Oscap/components/PolicySelector.tsx 48.71% <100.00%> (+0.66%) ⬆️
...steps/Review/components/AdvancedSettings/index.tsx 100.00% <100.00%> (ø)
...rd/steps/Review/components/ImageOverview/index.tsx 100.00% <100.00%> (ø)
...eWizard/steps/Oscap/components/ProfileSelector.tsx 87.65% <95.23%> (+6.85%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29382df...8efd0c3. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch 2 times, most recently from a489a7a to a44d907 Compare May 19, 2026 06:34
@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch 2 times, most recently from b62a901 to f2c7fe4 Compare May 20, 2026 11:14
@mgold1234 mgold1234 marked this pull request as ready for review May 20, 2026 11:39
@mgold1234 mgold1234 requested a review from a team as a code owner May 20, 2026 11:39
@mgold1234 mgold1234 changed the title Wizard: use placeholder styling for profile selector in dark mode (HMS-10638) Wizard: Replace typeahead profile selector with simple dropdown (HMS-10638) May 20, 2026
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src/Components/CreateImageWizard/steps/Oscap/components/ProfileSelector.tsx Outdated
@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch 9 times, most recently from dd92752 to 25c087f Compare May 27, 2026 10:46
@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch 4 times, most recently from 2d68c6b to a3571d9 Compare June 3, 2026 09:29
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.
@mgold1234 mgold1234 force-pushed the fix-profile-placeholder-dark-mode branch from a3571d9 to 8efd0c3 Compare June 3, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant