CONSOLE-4701: Add fontsize+theme control to YAML editor #15735
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@logonoff: This pull request references CONSOLE-4701 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @sg00dwin |
openshift-ci
bot
added
component/core
logonoff
changed the title
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
2 times, most recently
from
f57ad67 to
9e3915a
Compare
WalkthroughAdds persistent YAML editor settings (theme and font size) with a new useEditYamlSettings hook, redesigns EditYamlSettingsModal and accessibility, updates Edit YAML integration and CodeEditor props, adjusts BasicCodeEditor typing and prop-spread precedence, and updates localization strings and minor UI text. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
2 times, most recently
from
59fb5b4 to
a2e37f3
Compare
|
Docs Approver: PX Approver: |
|
@logonoff: GitHub didn't allow me to assign the following users: jseseCCS. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
|
/assign @yapei |
logonoff
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
from
a2e37f3 to
bee7f06
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
237-271: Guard against invalid numeric input for font size
onChangeparses the input withNumber(...)and writes it directly tofontSize. If the user types something non-numeric, this can becomeNaNand be propagated into the editor options.Consider clamping and ignoring invalid values, e.g.:
- onChange={(event) => setFontSize(Number((event.target as HTMLInputElement).value))} + onChange={(event) => { + const value = Number((event.target as HTMLInputElement).value); + if (Number.isNaN(value)) { + return; + } + setFontSize(Math.max(5, value)); + }}This keeps the stored value sane and respects the minimum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(3 hunks)frontend/packages/console-shared/src/constants/common.ts(1 hunks)frontend/public/components/edit-yaml.tsx(6 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(3 hunks)frontend/public/locales/en/public.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-shared/src/constants/common.tsfrontend/public/locales/en/public.jsonfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/public/components/edit-yaml.tsx
🔇 Additional comments (5)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (1)
23-56: BasicCodeEditor theme and options merge look correctUsing
ThemeContextforthemeand moving...props.editorProps/...props.optionsafter the defaults gives sensible overrides while preserving the console theme and mono font. No issues spotted here.frontend/public/locales/en/public.json (1)
913-925: New editor-settings i18n strings are consistent and scopedThe added strings for colour scheme, font size, and the “applied immediately” notice align with the new settings modal usage and are consistent with existing public.json patterns.
Also applies to: 931-931
frontend/packages/console-shared/src/constants/common.ts (1)
49-52: YAML editor theme/font-size setting keys are well-structuredThe new user-setting and localStorage keys for theme override and custom font size follow the existing naming and prefixing conventions and should be easy to reuse across components.
frontend/public/components/edit-yaml.tsx (1)
185-187: YAML editor settings integration is coherentUsing
useEditYamlSettingsto drivefontSize,theme, tooltips, and sticky scroll, plus wiring those viaoptions,editorProps, and theupdateOptionseffect, keeps the editor behavior in sync with user settings and the new modal. The fullscreenappendTohandling for the modal also looks correct.Also applies to: 766-769, 786-786, 805-812, 860-873
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
316-360: CentralizeduseEditYamlSettingshook is a good abstractionExposing
useEditYamlSettingsto aggregate theme, font size, tooltips, and sticky scroll fromuseUserSettingsCompatibilitysimplifies consumers likeedit-yaml.tsxand keeps the settings model in one place. The modal + hook arrangement should scale if additional YAML editor options are added later.Also applies to: 362-389
| interface ConfigModalItemProps { | ||
| title: string; | ||
| /** Icon rendered inside the configuration modal. */ | ||
| icon?: ReactNode; | ||
| /** Description of the configuration option. */ | ||
| description: string; | ||
| checked?: boolean; | ||
| setChecked: (checked: boolean) => void; | ||
| Icon?: React.ComponentType; | ||
| /** Title of the configuration option. We assume that titles are unique. */ | ||
| title: string; | ||
| /** | ||
| * Optional ID of the configuration option. Also used as a prefix for the ids of inner elements and the OUIA id. | ||
| * - `${ouiaId}-title` for the element which contains the title | ||
| * - `${ouiaId}-description` for the element which contains the description | ||
| */ | ||
| id?: string; | ||
| /** | ||
| * Slot to render inside the configuration modal. Remember to add `aria-labelledby` and `aria-describedby` props | ||
| * to the control inside the slot, pointing to the title and description ids respectively. | ||
| */ | ||
| slot?: ReactNode; | ||
| } | ||
|
|
||
| const ConfigModalItem: React.FCC<ConfigModalItemProps> = ({ | ||
| const ConfigModalItem: React.FunctionComponent<ConfigModalItemProps> = ({ | ||
| icon = <CogIcon />, | ||
| description, | ||
| title, | ||
| id = `ConfigModalItem-${title.replace(/\s+/g, '-').toLowerCase()}`, | ||
| slot, | ||
| }) => ( | ||
| <Flex | ||
| alignItems={{ default: 'alignItemsCenter' }} | ||
| justifyContent={{ default: 'justifyContentSpaceBetween' }} | ||
| spaceItems={{ default: 'spaceItemsMd' }} | ||
| id={id} | ||
| > | ||
| <FlexItem flex={{ default: 'flex_1' }}> | ||
| <Flex spaceItems={{ default: 'spaceItemsSm' }}> | ||
| <Icon isInline>{icon}</Icon> | ||
| <strong id={`${id}-title`} className="pf-v6-u-font-weight-bold"> | ||
| {title} | ||
| </strong> | ||
| </Flex> | ||
|
|
||
| <div id={`${id}-description`}>{description}</div> | ||
| </FlexItem> | ||
| <FlexItem alignSelf={{ default: 'alignSelfCenter' }}>{slot}</FlexItem> | ||
| </Flex> | ||
| ); |
There was a problem hiding this comment.
Fix unsupported ouia-id prop on ConfigModalItem
ConfigModalItemProps doesn’t define an ouia-id prop, and ConfigModalItem doesn’t forward unknown props, so the ouia-id usage in ThemeConfigItem and FontSizeConfigItem is both a TypeScript error and a no-op at runtime.
Either drop the prop or add a typed ouiaId prop and forward it to the root container. For example:
interface ConfigModalItemProps {
/** Icon rendered inside the configuration modal. */
icon?: ReactNode;
/** Description of the configuration option. */
description: string;
/** Title of the configuration option. We assume that titles are unique. */
title: string;
- /**
+ /**
* Optional ID of the configuration option. Also used as a prefix for the ids of inner elements and the OUIA id.
* - `${ouiaId}-title` for the element which contains the title
* - `${ouiaId}-description` for the element which contains the description
*/
id?: string;
+ /** Optional OUIA id forwarded to the root container. */
+ ouiaId?: string;
/**
@@
-const ConfigModalItem: React.FunctionComponent<ConfigModalItemProps> = ({
- icon = <CogIcon />,
- description,
- title,
- id = `ConfigModalItem-${title.replace(/\s+/g, '-').toLowerCase()}`,
- slot,
-}) => (
+const ConfigModalItem: React.FunctionComponent<ConfigModalItemProps> = ({
+ icon = <CogIcon />,
+ description,
+ title,
+ id = `ConfigModalItem-${title.replace(/\s+/g, '-').toLowerCase()}`,
+ ouiaId,
+ slot,
+}) => (
<Flex
@@
- id={id}
+ id={id}
+ ouiaId={ouiaId}
>And then update usages:
- <ConfigModalItem
+ <ConfigModalItem
key="colour-scheme"
title={t('Colour scheme')}
description={t('Select the colour scheme of the code editor')}
- ouia-id="ConfigModalItem-colour-scheme"
+ ouiaId="ConfigModalItem-colour-scheme"
@@
- <ConfigModalItem
+ <ConfigModalItem
key="font-size"
title={t('Font size')}
description={t('Adjust the font size of the code editor')}
- ouia-id="ConfigModalItem-font-size"
+ ouiaId="ConfigModalItem-font-size"Also applies to: 215-233, 247-271
🤖 Prompt for AI Agents
In frontend/public/components/modals/edit-yaml-settings-modal.tsx around lines
87-132 (and similarly for the other occurrences at 215-233 and 247-271),
ConfigModalItem currently lacks an ouiaId prop and does not forward unknown
props, so passing `ouia-id` is a TypeScript error and a no-op; add an optional
typed prop `ouiaId?: string` to ConfigModalItemProps and forward it to the root
container (the Flex element) as `ouiaId={ouiaId}` (or the prop name your OUIA
helper expects), then update any callers (ThemeConfigItem, FontSizeConfigItem,
etc.) to pass `ouiaId` (camelCase) instead of `ouia-id`. Ensure the prop is
optional and does not change existing behavior when omitted.
|
@logonoff I'd like to see one more time after changes. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(3 hunks)frontend/packages/console-shared/src/constants/common.ts(1 hunks)frontend/public/components/edit-yaml.tsx(6 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(3 hunks)frontend/public/locales/en/public.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/public/components/edit-yaml.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-shared/src/constants/common.tsfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/public/locales/en/public.json
🔇 Additional comments (6)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (2)
1-1: LGTM: Modern React import practices.The changes to import
FCanduseContextdirectly from React (rather than usingReact.FCandReact.useContext) follow current React best practices and improve code conciseness without any functional impact.Also applies to: 23-23, 25-25
44-56: LGTM: Props spreading order enables theme/fontsize overrides.The reordering of default properties before the spread operators (
themeandfontFamilynow precede...props?.editorPropsand...props?.options) is an intentional change that allows consumers to override the default theme and font family. This directly supports the PR objective of adding fontsize and theme control to the YAML editor.The
beforeMountcomposition remains correct, ensuring that console themes are always defined while allowing consumers to extend with their own setup logic.frontend/packages/console-shared/src/constants/common.ts (1)
49-52: YAML editor setting keys look consistent and correctThe new user setting and local storage keys follow the existing prefix and naming patterns for editor-related settings; no issues spotted.
frontend/public/locales/en/public.json (1)
913-925: New editor preference strings align with the settings UIThe added strings for color scheme, font size, and the immediate-apply notice are clear, consistent in tone with surrounding copy, and correctly keyed for reuse.
Also applies to: 931-931
frontend/public/components/modals/edit-yaml-settings-modal.tsx (2)
181-235: Overall YAML editor settings wiring and accessibility look solid
- Theme selection via
SimpleSelectand persisteduseUserSettingsCompatibilitystate is straightforward, and options are localized correctly.- Tooltip and sticky scroll switches reuse the shared
ConfigModalSwitchpattern with ARIA labels and descriptions tied to the item title/description.- The modal is correctly labeled/described via
aria-labelledby/aria-describedby, and theCodeEditorControltrigger advertisesaria-haspopup="dialog".useEditYamlSettingscentralizes retrieval of theme, font size, tooltips, and sticky scroll flags, which should simplify integration in the YAML editor.Aside from the font-size and
labelstyping nits already noted, this structure is clean and maintainable.Also applies to: 274-312, 316-360, 362-389
134-172: The review comment is based on incorrect assumptions and should be disregarded.The review claims the code violates TypeScript's
strictNullChecks, but the project'stsconfig.jsonhas"strict": false, which disables null checking. Additionally, PatternFly's Switch component'slabelprop is typed asReact.ReactNode, which acceptsundefined. The code compiles without error and functions correctly at runtime.While the interface design could be cleaner (declaring the fields as optional
enabled?: string), there is no actual type error to fix.Likely an incorrect or invalid review comment.
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
from
efafc08 to
04d03a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
237-272: Add input validation to onChange handler.The
onChangehandler on line 265 does not validate input, allowingNaNor0to be set when users clear the field or enter invalid text. This was previously flagged and marked as addressed, but the validation is still missing.Apply the recommended fix from the previous review:
- onChange={(event) => setFontSize(Number((event.target as HTMLInputElement).value))} + onChange={(event) => { + const raw = (event.target as HTMLInputElement).value; + const next = Number(raw); + if (!Number.isNaN(next)) { + setFontSize(Math.max(5, next)); + } + }}
🧹 Nitpick comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
362-389: Consider memoizing the return value of useEditYamlSettings.The hook returns a new object on every render, which could cause unnecessary re-renders in consuming components. While not critical (since
useUserSettingsCompatibilitylikely prevents excessive updates), memoizing the return value would be more efficient:+import { useMemo } from 'react'; export const useEditYamlSettings = () => { const [theme] = useUserSettingsCompatibility<'default' | 'dark' | 'light'>( OVERRIDE_YAML_EDITOR_THEME_USER_SETTING_KEY, OVERRIDE_YAML_EDITOR_THEME_LOCAL_STORAGE_KEY, 'default', true, ); const [fontSize] = useUserSettingsCompatibility<number>( CUSTOM_YAML_EDITOR_FONT_SIZE_USER_SETTING_KEY, CUSTOM_YAML_EDITOR_FONT_SIZE_LOCAL_STORAGE_KEY, 14, true, ); const [showTooltips] = useUserSettingsCompatibility<boolean>( SHOW_YAML_EDITOR_TOOLTIPS_USER_SETTING_KEY, SHOW_YAML_EDITOR_TOOLTIPS_LOCAL_STORAGE_KEY, true, true, ); const [stickyScrollEnabled] = useUserSettingsCompatibility<boolean>( SHOW_YAML_EDITOR_STICKY_SCROLL_USER_SETTING_KEY, SHOW_YAML_EDITOR_STICKY_SCROLL_LOCAL_STORAGE_KEY, true, true, ); - return { theme, fontSize, showTooltips, stickyScrollEnabled }; + return useMemo( + () => ({ theme, fontSize, showTooltips, stickyScrollEnabled }), + [theme, fontSize, showTooltips, stickyScrollEnabled], + ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(3 hunks)frontend/packages/console-shared/src/constants/common.ts(1 hunks)frontend/public/components/edit-yaml.tsx(6 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(3 hunks)frontend/public/locales/en/public.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/public/components/edit-yaml.tsx
- frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-shared/src/constants/common.tsfrontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/public/locales/en/public.json
🔇 Additional comments (2)
frontend/public/locales/en/public.json (1)
913-931: LGTM! Locale strings are clear and consistent.The new YAML editor settings strings are well-written and align with previous review feedback. The wording is concise and follows established patterns.
frontend/packages/console-shared/src/constants/common.ts (1)
49-52: LGTM! Constants follow established patterns.The new YAML editor constants are well-named and consistent with existing user settings and local storage keys in the file.
|
after after: 
cc @jseseCCS |
openshift-ci
bot
added
the
docs-approved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (1)
237-271: HardenFontSizeConfigItemagainst invalid/empty numeric inputThe
NumberInputonChangeat Line 265 currently does:onChange={(event) => setFontSize(Number((event.target as HTMLInputElement).value))}If the user clears the field or types non-numeric text,
Number(...)can yield0orNaN, which may get persisted viauseUserSettingsCompatibilityand then used as the editor font size, producing invalid styles or a near-invisible font size.Clamp and validate before updating state, e.g.:
- onChange={(event) => setFontSize(Number((event.target as HTMLInputElement).value))} + onChange={(event) => { + const raw = (event.target as HTMLInputElement).value; + const next = Number(raw); + if (!Number.isNaN(next)) { + setFontSize(Math.max(5, next)); + } + }}This keeps
fontSizesane and aligned with themin={5}constraint while still allowing free typing.
🧹 Nitpick comments (2)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (2)
174-180: Theme configuration component is sound; consider centralizing theme type
ThemeConfigItemcorrectly wiresuseUserSettingsCompatibilityto the theme user/local-storage keys and usesSimpleSelectwith accessiblearia-labelledby/aria-describedbypointing atConfigModalItemcontent. This looks good.For maintainability, you’re duplicating the
'default' | 'dark' | 'light'theme union here (implicitly) and again inuseEditYamlSettings(Line 364). Consider introducing a sharedtype YamlEditorTheme = 'default' | 'dark' | 'light';and using it both inuseEditYamlSettingsand in the settings hooks so they stay in sync over time.Also applies to: 181-235
316-360: Use explicit close behavior instead of toggling modal state inonCloseIn Line 332,
onClose={() => setIsModalOpen(!isModalOpen)}toggles the modal state rather than unconditionally closing. This works today but is brittle: ifonCloseever fires whileisModalOpenis alreadyfalse(or if multiple close events race), it can reopen the modal unexpectedly.Safer to set the state explicitly:
- onClose={() => setIsModalOpen(!isModalOpen)} + onClose={() => setIsModalOpen(false)}Everything else in the modal wiring (IDs, ARIA attributes, composition of the config items, and the
CodeEditorControltrigger) looks solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/public/components/modals/edit-yaml-settings-modal.tsx(3 hunks)frontend/public/locales/en/public.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/public/locales/en/public.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/modals/edit-yaml-settings-modal.tsx
🔇 Additional comments (4)
frontend/public/components/modals/edit-yaml-settings-modal.tsx (4)
1-80: Icons and imports look consistent and low-riskImports and the four Font Awesome–based icons created via
createIconare scoped and used appropriately in the config items; licensing headers are present and the setup is straightforward with no apparent performance or correctness concerns.
274-312: Tooltip and sticky-scroll switches are wired correctly
TooltipConfigItemandStickyScrollConfigItemconsistently useConfigModalSwitchanduseUserSettingsCompatibilitywith the correct user/local-storage keys, and the switchonChangehandlers simply forward the booleancheckedvalue to the corresponding setters. The icon choices and descriptions are clear and the a11y setup (viaConfigModalSwitch) looks fine.
362-389:useEditYamlSettingshook cleanly centralizes editor settings
useEditYamlSettingsneatly aggregatestheme,fontSize,showTooltips, andstickyScrollEnabledviauseUserSettingsCompatibility, with appropriate generics and defaults. This should simplify consumers like the YAML editor by providing a single, typed source of truth for configuration. No issues spotted here.
87-132: FixConfigModalSwitchProps.labelstyping mismatchThe interface requires
enabledanddisabledasstringtype (lines 140-142), but the default value at line 153 assignsundefinedto both, causing a TypeScript type error. The Switch component's label prop accepts ReactNode, so relaxing the type to optional ReactNode is appropriate.Update the interface and default as suggested:
interface ConfigModalSwitchProps extends Omit<ConfigModalItemProps, 'slot'> { /** Flag indicating whether the option is enabled or disabled. */ isChecked?: SwitchProps['isChecked']; /** onChange handler for the switch. */ onChange?: SwitchProps['onChange']; - /** Labels for the enabled and disabled states of the switch. */ + /** Labels for the enabled and disabled states of the switch (optional). */ labels?: { - enabled: string; - disabled: string; + enabled?: ReactNode; + disabled?: ReactNode; }; } const ConfigModalSwitch: React.FunctionComponent<ConfigModalSwitchProps> = ({ icon = <CogIcon />, description, title, id = `ConfigModalSwitch-${title.replace(/\s+/g, '-').toLowerCase()}`, isChecked = false, onChange, - labels = { enabled: undefined, disabled: undefined }, + labels = {}, }) => (This eliminates the type violation while maintaining safety for all call sites (TooltipConfigItem, StickyScrollConfigItem) which omit the
labelsprop entirely.Likely an incorrect or invalid review comment.
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
from
62e7c8e to
18d3dc7
Compare
logonoff
force-pushed
the
CONSOLE-4701-codeeditor
branch
from
18d3dc7 to
0043dd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (7)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx(3 hunks)frontend/packages/console-shared/src/constants/common.ts(1 hunks)frontend/public/components/edit-yaml.tsx(6 hunks)frontend/public/components/modals/edit-yaml-settings-modal.tsx(3 hunks)frontend/public/components/modals/expand-pvc-modal.tsx(1 hunks)frontend/public/components/monitoring/receiver-forms/slack-receiver-form.tsx(2 hunks)frontend/public/locales/en/public.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/public/components/monitoring/receiver-forms/slack-receiver-form.tsx
- frontend/public/components/edit-yaml.tsx
- frontend/packages/console-shared/src/constants/common.ts
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/modals/edit-yaml-settings-modal.tsxfrontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsxfrontend/public/components/modals/expand-pvc-modal.tsxfrontend/public/locales/en/public.json
🔇 Additional comments (15)
frontend/packages/console-shared/src/components/editor/BasicCodeEditor.tsx (2)
1-1: LGTM: Modern React conventions applied.The refactoring from namespace import (
import * as React) to named imports (FC,useContext) is a good practice that aligns with modern React conventions and improves tree-shaking.Also applies to: 23-23, 25-25
44-56: No concerns to address—this is a feature enablement, not a breaking change.Based on verification of all BasicCodeEditor consumers in the codebase:
- resource-sidebar-samples.tsx only passes options with lineHeight and UI settings, not theme overrides
- CodeEditor wrapper merges options with spread pattern for consumer overrides, and editorProps are spread via
{...props}- edit-yaml.tsx attempts to override theme via editorProps, which now works correctly with the new precedence
- No existing consumers are relying on enforced defaults
The precedence change—moving theme and fontFamily before the spread—enables the new capability to customize these values rather than blocking it. This aligns with the git history showing
CONSOLE-4701: Add fontsize+theme to YAML editoras a new feature. All consumers are compatible with this change.frontend/public/components/modals/expand-pvc-modal.tsx (1)
62-63: LGTM! UI text improvements align with style guidelines.The wording changes improve clarity and follow documentation best practices: using positive phrasing ("must be at least" instead of "can't be less than") and setting clearer expectations ("might take some time to complete" instead of "can be a time-consuming process").
frontend/public/components/modals/edit-yaml-settings-modal.tsx (8)
35-79: LGTM! Custom icons properly implemented.The custom icon components follow the PatternFly
createIconpattern correctly and include proper Font Awesome Free license attribution.
87-132: LGTM! ConfigModalItem component is well-structured.The component provides a clean, reusable layout for configuration items with proper ID generation and accessibility support through
aria-labelledbyandaria-describedbypatterns.
134-172: LGTM! ConfigModalSwitch provides good abstraction.The component properly wraps
ConfigModalItemwith Switch-specific functionality and maintains accessibility through correct ARIA attribute forwarding.
183-237: LGTM! ThemeConfigItem correctly implements theme selection.The component properly uses
useUserSettingsCompatibilityfor persistence,useMemofor performance, and maintains accessibility with appropriate ARIA attributes.
276-294: LGTM! Clean refactor to ConfigModalSwitch.The component correctly migrates to the new
ConfigModalSwitchabstraction while maintaining the same functionality.
296-314: LGTM! Consistent refactor with improved naming.The component correctly uses
ConfigModalSwitchand the variable naming (stickyScrollEnabled) addresses previous review feedback.
316-362: LGTM! Modal component properly structured.The modal correctly implements accessibility patterns, state management, and composes all configuration items cleanly.
364-391: LGTM! Hook provides clean API for settings access.The
useEditYamlSettingshook correctly aggregates all YAML editor settings into a single, consumable interface, avoiding prop drilling and providing a clear API for consumers.frontend/public/locales/en/public.json (4)
913-921: LGTM! Theme strings are clear and approved.The theme-related strings correctly reflect the in-app setting behavior (as clarified in past reviews) and maintain consistency with the rest of the UI.
922-931: LGTM! Editor strings follow documentation best practices.All updates align with previous documentation review feedback: comma series for clarity in the tooltip description, standard UI verb "Pin" instead of "Stick", and present tense "Changes apply immediately" per style guidelines.
934-934: LGTM! PVC message improvements match UI changes.The updated text uses positive phrasing and sets clearer expectations, consistent with the corresponding modal component changes and documentation style guidelines.
1072-1074: LGTM! Slack label improvements follow style guidelines.The updated labels are more concise and direct, removing periods from noun phrase labels per documentation standards.
|
Review Request |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jseseCCS, Leo6Leo, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Only one minor issue found during testing, the Theme dropdown list got cut at right on Safari |
|
/verified by @yapei |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
8 participants
Dependent on review from patternfly/patternfly-react#12113
after: