feat(CodeEditor): add isHighContrastTheme#12384
Open
logonoff wants to merge 1 commit intopatternfly:mainfrom
Open
feat(CodeEditor): add isHighContrastTheme#12384logonoff wants to merge 1 commit intopatternfly:mainfrom
logonoff wants to merge 1 commit intopatternfly:mainfrom
Conversation
Collaborator
|
Preview: https://pf-react-pr-12384.surge.sh A11y report: https://pf-react-pr-12384-a11y.surge.sh |
WalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx (2)
160-161: Document the interaction withisDarkThemein the JSDoc.The behavior depends on
isDarkTheme(selectinghc-blackvshc-light), but the doc comment doesn't surface that. Consumers reading IDE hints will likely miss the dependency.📝 Proposed JSDoc clarification
- /** Flag indicating the editor is styled using monaco's high contrast themes. */ + /** Flag indicating the editor uses Monaco's built-in high-contrast themes. When enabled, + * `isDarkTheme` selects between `hc-black` (dark) and `hc-light` (light), overriding the + * default PatternFly themes. */ isHighContrastTheme?: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx` around lines 160 - 161, Update the JSDoc for the isHighContrastTheme prop to explicitly state it depends on isDarkTheme: document that when isHighContrastTheme is true the component will select Monaco's "hc-black" theme if isDarkTheme is true and "hc-light" if isDarkTheme is false (and that isDarkTheme takes precedence in choosing the specific high-contrast variant); reference the isHighContrastTheme and isDarkTheme symbols so consumers can see the interaction in IDE hints.
468-473:useMemois unnecessary for this computation.The branches return string literals from two boolean inputs — recomputing per render is effectively free, and the memoized value is consumed as a Monaco prop string, so referential stability buys nothing. A plain ternary keeps the code simpler.
♻️ Proposed simplification
- const theme = useMemo(() => { - if (isHighContrastTheme) { - return isDarkTheme ? 'hc-black' : 'hc-light'; - } - return isDarkTheme ? 'pf-v6-theme-dark' : 'pf-v6-theme-light'; - }, [isHighContrastTheme, isDarkTheme]); + const theme = isHighContrastTheme + ? (isDarkTheme ? 'hc-black' : 'hc-light') + : (isDarkTheme ? 'pf-v6-theme-dark' : 'pf-v6-theme-light');If you keep
useMemo,useMemocan be removed from the imports on line 1 if no other usage remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx` around lines 468 - 473, The computed theme value using useMemo can be simplified: replace the useMemo-based assignment to the theme constant inside CodeEditor.tsx (which currently references isHighContrastTheme and isDarkTheme) with a direct ternary expression that returns the same string literals, and remove useMemo from the imports if no other usages remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx`:
- Around line 160-161: Update the JSDoc for the isHighContrastTheme prop to
explicitly state it depends on isDarkTheme: document that when
isHighContrastTheme is true the component will select Monaco's "hc-black" theme
if isDarkTheme is true and "hc-light" if isDarkTheme is false (and that
isDarkTheme takes precedence in choosing the specific high-contrast variant);
reference the isHighContrastTheme and isDarkTheme symbols so consumers can see
the interaction in IDE hints.
- Around line 468-473: The computed theme value using useMemo can be simplified:
replace the useMemo-based assignment to the theme constant inside CodeEditor.tsx
(which currently references isHighContrastTheme and isDarkTheme) with a direct
ternary expression that returns the same string literals, and remove useMemo
from the imports if no other usages remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ba3d96d-1801-4009-8968-8da952588ef4
📒 Files selected for processing (4)
packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsxpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditor.mdpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditorBasic.tsxpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditorConfigurationModal.tsx
Adds a new prop, `isHighContrastTheme`, which switches the CodeEditor over to the monaco-provided `hc-black` and `hc-light` themes (based on the value of `isDarkTheme`) when enabled. A custom PatternFly variant of the hc-black and hc-light themes can come in a follow up, depending on the direction design wants to take on this
logonoff
force-pushed
the
12383-codeeditor-hc
branch
from
222a78d to
e963ba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx`:
- Around line 468-473: The computed theme in CodeEditor is getting overridden by
caller-supplied editorProps.theme because editorProps is spread after theme; to
fix, ensure the component's computed theme cannot be overridden by moving the
spread before theme or by merging theme into editorProps (e.g., create
mergedEditorProps = { ...editorProps, theme } and pass mergedEditorProps) so the
local variable theme (from useMemo) always wins; update usages around the
CodeEditor render where theme and editorProps are applied (also apply same
change at the other occurrence noted near lines 592-593).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4186b34-70f2-4d20-a806-b72a95095b1f
📒 Files selected for processing (4)
packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsxpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditor.mdpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditorBasic.tsxpackages/react-code-editor/src/components/CodeEditor/examples/CodeEditorConfigurationModal.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-code-editor/src/components/CodeEditor/examples/CodeEditor.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-code-editor/src/components/CodeEditor/examples/CodeEditorConfigurationModal.tsx
- packages/react-code-editor/src/components/CodeEditor/examples/CodeEditorBasic.tsx
Comment on lines
+468
to
+473
| const theme = useMemo(() => { | ||
| if (isHighContrastTheme) { | ||
| return isDarkTheme ? 'hc-black' : 'hc-light'; | ||
| } | ||
| return isDarkTheme ? 'pf-v6-theme-dark' : 'pf-v6-theme-light'; | ||
| }, [isHighContrastTheme, isDarkTheme]); |
There was a problem hiding this comment.
Keep the computed theme from being overridden.
{...editorProps} currently comes after theme={theme}, so any caller-supplied editorProps.theme will win and can silently bypass isHighContrastTheme.
🔧 Suggested fix
<Editor
height={height === '100%' ? undefined : height}
width={width}
language={language}
value={value}
options={editorOptions}
overrideServices={overrideServices}
onChange={onModelChange}
onMount={editorDidMount}
loading={loading}
- theme={theme}
{...editorProps}
+ theme={theme}
beforeMount={editorBeforeMount}
/>Also applies to: 592-593
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-code-editor/src/components/CodeEditor/CodeEditor.tsx` around
lines 468 - 473, The computed theme in CodeEditor is getting overridden by
caller-supplied editorProps.theme because editorProps is spread after theme; to
fix, ensure the component's computed theme cannot be overridden by moving the
spread before theme or by merging theme into editorProps (e.g., create
mergedEditorProps = { ...editorProps, theme } and pass mergedEditorProps) so the
local variable theme (from useMemo) always wins; update usages around the
CodeEditor render where theme and editorProps are applied (also apply same
change at the other occurrence noted near lines 592-593).
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
2 participants
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.
What: Closes #12383
Adds a new prop,
isHighContrastTheme, which switches the CodeEditor over to the monaco-providedhc-blackandhc-lightthemes (based on the value ofisDarkTheme) when enabled.A custom PatternFly variant of the hc-black and hc-light themes can come in a follow up, depending on the direction design wants to take on this
Summary by CodeRabbit