Changed various UI components. #1451
Conversation
Ref https://linear.app/ghost/issue/PLG-355/improve-cta-card-settings-panel - Updated ButtonGroup styles. - Added a button-style image upload placeholder to MediaPlaceholder. Also updated the VideoCard to follow the same style. - Refactored ColorOptionButtons and ColorPicker to display swatches and colorpicker inside toolbar. This also affects the callout, signup and header cards and the image-background selector. - Added pink and purple as background colors to the CTA card. - Changed CTA card link styles to currentColor instead of accent color – both for the label and the body text.
WalkthroughThis PR spans multiple packages to update and enhance both functionality and UI components. It revises the default properties in the CallToAction node—changing the button display, text, and color—to align the rendered output and associated tests with new expectations. In addition, several new components and beta variants (e.g., ImageUploadSwatch, ButtonGroupBeta, ColorPickerBeta, and MediaPlaceholderBeta) have been introduced to expand the UI toolkit. Enhancements include the addition of new hooks (such as useClickOutside), updates to existing hooks for improved error handling, and refinements in styling and configuration (e.g., a new box-shadow utility and updated CSS rules). Test suites across unit and end-to-end tests have been adjusted to reflect these changes by streamlining interactions, assertions, and background color settings. Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.stories.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ronaldlangeveld
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (1)
173-196: Consider deriving color PropTypes from CALLOUT_COLORS.Instead of manually listing color options, consider deriving them from the CALLOUT_COLORS object keys for better maintainability.
- color: PropTypes.oneOf(['white', 'grey', 'blue', 'green', 'yellow', 'red', 'pink', 'purple', 'accent']), + color: PropTypes.oneOf(Object.keys(CALLOUT_COLORS)),packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
9-9: Ensure documentation of thechildrenprop.
The newchildrenprop is helpful for injecting additional UI elements into the picker, but consider documenting its usage or providing a default behavior to help maintain clarity and consistency.
193-196: Consider giving users the option to auto-close the picker.
ThehandleColorPickerChangefunction currently avoids closing the popover, which can be helpful for making repeated selections. However, some users may expect it to close automatically upon color selection.
199-297: Avoid repetition of the conic gradient code.
The repeated conic gradient block (lines 210-216 and again at 276-282) could be factored out into a small helper or CSS class to improve readability and reduce duplication.- <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(...)', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - ... - }} /> + <div className="absolute inset-0 rounded-full p-[3px] my-conic-gradient" />packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (6)
20-30: Ensure usage oftypeprop is documented.
Although changing theCardTextcomponent to handle atypeprop is straightforward, consider adding documentation or a JSDoc comment indicating whentype='button'should be used. This will help maintain clarity for future contributors.
49-56: Consider simplifying top-level conditionals.
The classes for'button'vs.'border'usage might be compressed if the logic continues to grow. A dedicated helper or mapping object might improve readability and maintainability.
67-71: Styling consistency for small vs xsmall.
Ensure the text alignment and spacing remain consistent across all size variants, especially if new sizes or conditions are introduced.
87-88: Check container usage fordata-testid.
This is referencing the entire container as'media-placeholder'. If you need more granular UI tests, consider adding more descriptive naming for nested elements.
96-116: Separate the inline button styles from code logic.
Here, there's a direct style usage for'px-3 py-1'. To keep styling consolidated, consider using a helper class or reusingbuttonClasseswith extended conditions. Maintaining consistent logic prevents style drift.
117-139: Avoid large inline code blocks.
Both the “button” and “else” blocks contain a fair amount of inline HTML and classes. If you find the logic grows, consider refactoring them into smaller functional components with well-defined responsibilities.packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (1)
5-24: Well-structured new component and stylings.
ThisImageUploadSwatchis succinct and readable. Theoutline-greenstyle effectively highlights when the background image is active. If you plan to scale the palette, consider a more themable or dynamic approach to the outline color.packages/koenig-lexical/test/utils/background-color-helper.js (1)
1-28: Consider adding error handling for element locators.The implementation is well-structured and modular. However, it could benefit from error handling for cases where elements are not found.
Consider wrapping the locator operations in try-catch blocks:
export async function cardBackgroundColorSettings(page, {cardColorPickerTestId, customColor, colorTestId, findByColorTitle, imageUploadId, fireColorSetting = true}) { if (fireColorSetting) { - const colorSetting = page.locator(`[data-testid="${cardColorPickerTestId}"]`); - const colorButton = colorSetting.locator('button'); - await colorButton.click(); + try { + const colorSetting = page.locator(`[data-testid="${cardColorPickerTestId}"]`); + const colorButton = colorSetting.locator('button'); + await colorButton.click(); + } catch (error) { + throw new Error(`Failed to interact with color picker: ${error.message}`); + } } // Similar error handling for other operations... }packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (1)
10-10: LGTM! Consider enhancing accessibility.The styling updates improve visual hierarchy and maintain dark mode support. The active state is now more prominent.
Consider adding ARIA attributes for better screen reader support:
- <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white"> + <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white" role="toolbar" aria-label="Button group options">className={`group relative flex h-7 w-8 cursor-pointer items-center justify-center rounded-lg text-black dark:text-white dark:hover:bg-grey-900 ${isActive ? 'border border-grey-300 bg-white shadow-xs dark:bg-grey-900' : '' } ${Icon ? '' : 'text-[1.3rem] font-bold'}`} + aria-pressed={isActive}Also applies to: 36-36
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
17-25: Consider memoizing the gradient style object.The gradient style object is recreated on every render. Consider memoizing it for better performance.
+const gradientStyle = { + background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', + WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + WebkitMaskComposite: 'xor', + mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + maskComposite: 'exclude' +}; export function ColorOptionButtons({buttons = [], selectedName, onClick}) { // ... - <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - WebkitMaskComposite: 'xor', - mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - maskComposite: 'exclude' - }} /> + <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={gradientStyle} />packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
190-191: Update test assertions to match new implementation.The test assertions have been updated to check for
shadow-xsclass instead ofbg-grey-150, but the test description still refers to "size". Consider updating the test description to better reflect what's being tested.- test('can change the size', async function () { + test('can change the shadow size', async function () {packages/koenig-lexical/test/e2e/cards/signup-card.test.js (1)
364-365: Remove redundant comments.The commented code and redundant comments should be removed to maintain clean test code.
- // data-testid="settings-panel" - - await page.locator('[data-testid="settings-panel"]').click(); // click here to close the colour swatch + await page.locator('[data-testid="settings-panel"]').click();Also applies to: 374-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(2 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/ImageUploadForm.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx(4 hunks)packages/koenig-lexical/src/components/ui/MediaUploader.jsx(3 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(8 hunks)packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v1/HeaderCard.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(2 hunks)packages/koenig-lexical/src/components/ui/cards/VideoCard.jsx(2 hunks)packages/koenig-lexical/src/styles/components/kg-prose.css(1 hunks)packages/koenig-lexical/tailwind.config.cjs(1 hunks)packages/koenig-lexical/test/e2e/cards/callout-card.test.js(3 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(4 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(9 hunks)packages/koenig-lexical/test/e2e/cards/signup-card.test.js(10 hunks)packages/koenig-lexical/test/utils/background-color-helper.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/koenig-lexical/src/components/ui/cards/HeaderCard/v1/HeaderCard.jsx
🔇 Additional comments (52)
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (3)
9-35: LGTM! Well-structured color constants with proper dark mode support.The color constants are well-organized, with consistent patterns and proper handling of dark mode. The special case for accent color links is also handled appropriately.
37-83: LGTM! Improved color visibility with consistent border styling.The opacity increase from 15% to 20% enhances visibility while maintaining a subtle appearance. The border colors are consistently applied across all swatches, providing a cohesive look in both light and dark modes.
85-171: LGTM! Well-structured component with proper state management.The component follows React best practices with:
- Proper use of hooks for state management
- Clean handling of emoji picker state
- Clear separation of concerns between display and settings panel
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
2-2: Nice addition of hooks.
Importing additional React hooks (e.g.,useCallback,useState) looks appropriate here and aligns well with the usage throughout the file.
78-99: Validate event-blocking logic for accessibility.
UsingonMouseDownandonTouchStartto callstopPropagationprevents the color picker from closing when clicked internally, which is desirable behavior. However, remember to verify accessibility scenarios (e.g., keyboard/screen reader users) to ensure these added event handlers don’t create navigation hurdles.
138-179: Good modular approach for color popover state.
The introduction of separate states (isOpen,showColorPicker,showChildren) and a dedicatedhandleDocumentClickfor closing the menu on outside clicks is a standard and effective pattern for managing popovers.packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (6)
9-9: Nice addition ofclsxfor conditional class handling.
This import is appropriate and simplifies the code.
37-37: Verify that alltypeenum values are handled.
The newtypeprop is introduced here. Double-check whether your code can gracefully handle future expansions (e.g.,'gallery'or others). If not, consider either refining the prop type or providing graceful fallbacks for unrecognized values.
58-65: Good usage ofclsxchaining.
This makes the icon styling easier to maintain. future changes can simply append or remove conditions here.
79-83: Enhanced error styling.
TheerrorClassesstructure is clear and ensures consistent styling. Consider whether more descriptive error messages are needed or if an icon might help highlight them further.
91-94: Eye-catching drag-over text.
The placeholder text "Drop 'em like it's hot" is fun. Ensure this is appropriate for all user contexts or consider i18n if needed.
151-151: PropType expansion fortype.
You've updated the propTypes to allow'image'and'button'. Ensure that any additional usage stays synchronized throughout the codebase (e.g., if'gallery'or'file'also become valid in the future).packages/koenig-lexical/src/components/ui/ImageUploadForm.jsx (1)
14-15:stopPropagation()usage check.
Stopping propagation is valid for preventing accidental clicks from bubbling. However, be mindful if you rely on any parent or global event listeners for distinct features (e.g., close modals, triggers). Confirm that halting the bubble won’t cause unexpected UI side effects.packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (1)
1-4: Comprehensive imports for new component.
Usingclsxand your custom icons/Tooltip is consistent with the project’s design system. Good approach.packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
11-14:❓ Verification inconclusive
LGTM! Consider version bump for breaking changes.
The default value changes improve the out-of-the-box experience. However, changing
showButtonfromfalsetotrueis a breaking change that might affect existing implementations.Please verify if this change requires a version bump by checking the package's versioning policy:
🏁 Script executed:
#!/bin/bash # Check current version and recent changes cat package.json | jq '.version' git log -1 --pretty=format:%BLength of output: 37
The root package.json returns a null version, indicating that versioning may be managed per package in this monorepo. Please run the above script to check the version in the specific package file. Once that information is confirmed according to the package’s versioning policy, verify whether the breaking change—changing
showButtonfromfalsetotrue—requires a version bump.Rewritten Review Comment:
Action Required: Verify package versioning for breaking change
The changes in the default values enhance the out-of-the-box experience. However, note that switching
showButtonfromfalsetotrueis a breaking change that might impact current integrations. Since the root package.json does not specify a version (it returnednull), please verify the version set inpackages/kg-default-nodes/package.jsonand confirm if a version bump is warranted by the package's versioning policy.
🏁 Script executed:
[run_scripts] #!/bin/bash if [ -f packages/kg-default-nodes/package.json ]; then echo "Checking version in packages/kg-default-nodes/package.json" cat packages/kg-default-nodes/package.json | jq '.version' else echo "Error: packages/kg-default-nodes/package.json not found" fiLength of output: 367
Action Required: Confirm appropriate version bump for breaking change
The updated defaults improve the initial experience, but note that changing
showButtonfromfalsetotrueis a breaking change that could impact existing integrations. The package's current version is"1.4.0"(as determined frompackages/kg-default-nodes/package.json). Please verify that this breaking change has been accompanied by an appropriate version bump in accordance with your semantic versioning policy.packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
7-9: LGTM! Clean state management implementation.The state management is well-implemented with proper initialization and selector for the selected button.
packages/koenig-lexical/src/components/ui/MediaUploader.jsx (1)
20-20: LGTM! Thetypeprop enhances the component's flexibility.The addition of the
typeprop with proper PropTypes validation aligns with the PR objectives to introduce a button-style image upload placeholder.Also applies to: 68-68, 125-125
packages/koenig-lexical/tailwind.config.cjs (1)
94-94: LGTM! The new box shadow utility enhances UI depth.The addition of the
xsbox shadow utility with a subtle shadow effect aligns with the PR objectives to enhance UI components.packages/koenig-lexical/src/components/ui/cards/VideoCard.jsx (1)
102-102: LGTM! The MediaUploadSetting changes improve user guidance.The addition of the 'Upload' description and button-style type enhances the user interface and aligns with the PR objectives.
Also applies to: 112-112
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (2)
246-273: LGTM! The ColorPickerSetting enhancements improve flexibility.The addition of
customToolbarContentandchildrenprops, along with simplified color picker integration, enhances component customization.
275-306: LGTM! The MediaUploadSetting layout improvements enhance adaptability.The conditional classes based on
typeandstackedprops provide better layout control, aligning with the PR objectives for UI enhancements.packages/koenig-lexical/test/e2e/cards/callout-card.test.js (4)
2-2: LGTM! Importing the background color helper.The addition of the
cardBackgroundColorSettingsimport improves code reusability by centralizing color setting logic.
128-136: Consider keeping the color picker test.While the test is commented out, it might be valuable to keep a test that verifies all colors are rendered in the picker, even if the implementation details have changed.
Do you want me to help generate an updated version of this test using the new
cardBackgroundColorSettingshelper?
138-144: LGTM! Improved test clarity and maintainability.The test has been updated to:
- Use a more descriptive name
- Utilize the centralized
cardBackgroundColorSettingshelper
315-319: LGTM! Improved test maintainability.The test has been updated to use the centralized
cardBackgroundColorSettingshelper, making it more maintainable.packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (7)
24-26: LGTM! Added new color options.The addition of pink and purple colors enhances the customization options available to users.
48-78: LGTM! Improved color contrast.The changes to color opacity values (from
/15to/20) improve visibility and contrast.
146-152: LGTM! Added test ID for better testability.The addition of the
dataTestIdprop improves the component's testability.
164-164: LGTM! Improved upload button UX.The changes to the
MediaUploadSettingcomponent:
- Add a descriptive label with
desc='Upload'- Use a more prominent button style with
type='button'Also applies to: 170-170
254-254: LGTM! Improved text styling.The updates to text styling classes:
- Add
text-prettyfor better text rendering- Use consistent color classes
Also applies to: 297-297
349-349: LGTM! Updated prop types.The
colorprop type has been updated to include the new pink and purple color options.
383-383: LGTM! Improved default UX.Setting
showButtontotrueby default improves the initial user experience by making the call-to-action more prominent.packages/kg-default-nodes/test/nodes/call-to-action.test.js (3)
86-88: LGTM! Updated test for new button visibility default.The test now correctly verifies that
showButtonistrueby default before testing the toggle functionality.
90-92: LGTM! Updated test for new button text default.The test now correctly verifies that
buttonTextdefaults to 'Learn more' before testing the change functionality.
102-104: LGTM! Updated test for new button color default.The test now correctly verifies that
buttonColordefaults to 'black' before testing the change functionality.packages/koenig-lexical/test/e2e/cards/cta-card.test.js (5)
122-129: LGTM! Improved button visibility testing.The test now correctly verifies that:
- The button is visible by default
- All button settings are visible by default
131-141: LGTM! Improved toggle testing.The test now properly verifies both the visibility and invisibility states of the button when toggled.
218-220: LGTM! Improved color change testing.The tests now use the centralized
cardBackgroundColorSettingshelper for better maintainability.Also applies to: 228-229, 236-237, 245-246
315-317: LGTM! Added tests for new colors.The test suite now includes verification of the new pink and purple color options.
323-323: LGTM! Improved color class testing.The test now:
- Verifies the absence of all color classes initially
- Uses a more maintainable approach to color selection
Also applies to: 325-326
packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (4)
19-19: LGTM! Improved modularity with ImageUploadSwatch.The import of ImageUploadSwatch enhances code reusability by extracting common image upload functionality into a dedicated component.
231-235: LGTM! Well-structured event handler.The
onBackgroundImageClickHandlerfunction properly manages state transitions:
- Shows background image
- Expands background color picker
- Collapses button color picker
413-417: LGTM! Clean implementation of ImageUploadSwatch.The ImageUploadSwatch component is properly integrated with required props for background image handling.
446-472: LGTM! Improved conditional rendering.The MediaUploadSetting is now properly conditionally rendered based on layout and showBackgroundImage state, with comprehensive props for image handling.
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (2)
415-417: LGTM! Improved color picker state management.The background color picker now expands when selecting the background image option, providing better UX by showing relevant settings immediately.
452-478: LGTM! Enhanced conditional rendering.The MediaUploadSetting is properly conditionally rendered with comprehensive props for image handling.
packages/koenig-lexical/test/e2e/cards/header-card.test.js (2)
3-3: LGTM! Enhanced test maintainability.The import of cardBackgroundColorSettings helper improves test maintainability by centralizing color setting logic.
226-227: LGTM! Improved test readability.The background color tests now use the cardBackgroundColorSettings helper, making the tests more maintainable and easier to understand.
Also applies to: 232-233, 238-239
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (3)
3-3: LGTM! Enhanced test maintainability.The import of cardBackgroundColorSettings helper improves test maintainability by centralizing color setting logic.
206-209: LGTM! Clean test implementation.The button color test now uses the cardBackgroundColorSettings helper with clear parameters.
232-236: LGTM! Improved test readability.The background color test now uses the cardBackgroundColorSettings helper with clear parameters.
packages/koenig-lexical/src/styles/components/kg-prose.css (1)
984-991: CTA Styling Implementation is Accurate and Consistent.The new CSS rules for
.koenig-lexical-cta-label aand.koenig-lexical-cta-text acorrectly apply the specified text colors and underline decoration for normal and dark modes using Tailwind’s@applydirective. This aligns well with the updated UI requirements for call-to-action elements described in the PR objectives. Please verify these styles in both light and dark mode contexts to ensure complete visual consistency.
| const buttonClasses = clsx( | ||
| 'group flex cursor-pointer select-none items-center justify-center', | ||
| size === 'xsmall' && 'p-4', | ||
| size !== 'xsmall' && 'flex-col p-20' | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential duplication of button styling code.
This buttonClasses definition partially overlaps with the inline version in the type === 'button' condition below. To keep the codebase DRY, consider consolidating them.
| {/* Color options popover */} | ||
| {isOpen && ( | ||
| <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"> | ||
| <div className="flex"> | ||
| <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"> | ||
| {buttons.map(({label, name, color}) => ( | ||
| name !== 'image' ? | ||
| <ColorButton | ||
| key={`${name}-${label}`} | ||
| color={color} | ||
| label={label} | ||
| name={name} | ||
| selectedName={selectedName} | ||
| onClick={(title) => { | ||
| onClick(title); | ||
| setIsOpen(false); | ||
| }} | ||
| /> | ||
| : | ||
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | ||
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | ||
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | ||
| </span> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for the color options popover.
The color options popover needs proper ARIA attributes for better accessibility.
- {isOpen && (
- <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow">
+ {isOpen && (
+ <div
+ className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"
+ role="dialog"
+ aria-label="Color options"
+ >- <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white">
+ <ul
+ className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"
+ role="listbox"
+ aria-label="Available colors"
+ >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {/* Color options popover */} | |
| {isOpen && ( | |
| <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"> | |
| <div className="flex"> | |
| <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"> | |
| {buttons.map(({label, name, color}) => ( | |
| name !== 'image' ? | |
| <ColorButton | |
| key={`${name}-${label}`} | |
| color={color} | |
| label={label} | |
| name={name} | |
| selectedName={selectedName} | |
| onClick={(title) => { | |
| onClick(title); | |
| setIsOpen(false); | |
| }} | |
| /> | |
| : | |
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | |
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | |
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | |
| </span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| </div> | |
| )} | |
| {/* Color options popover */} | |
| {isOpen && ( | |
| <div | |
| className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow" | |
| role="dialog" | |
| aria-label="Color options" | |
| > | |
| <div className="flex"> | |
| <ul | |
| className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white" | |
| role="listbox" | |
| aria-label="Available colors" | |
| > | |
| {buttons.map(({label, name, color}) => ( | |
| name !== 'image' ? | |
| <ColorButton | |
| key={`${name}-${label}`} | |
| color={color} | |
| label={label} | |
| name={name} | |
| selectedName={selectedName} | |
| onClick={(title) => { | |
| onClick(title); | |
| setIsOpen(false); | |
| }} | |
| /> | |
| : | |
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | |
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | |
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | |
| </span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| </div> | |
| )} |
| @@ -0,0 +1,28 @@ | |||
| export async function cardBackgroundColorSettings(page, {cardColorPickerTestId, customColor, colorTestId, findByColorTitle, imageUploadId, fireColorSetting = true}) { | |||
There was a problem hiding this comment.
I think the naming could do with some work here as it took me a while to work out what this helper is doing when looking at the tests and the options weren't clear
fireColorSetting-fireis usually used for events so it sounds like internal behaviours are leaking into the naming, it would be better if this describes what is actually happening as far as the tester is concernedfindByColorTitle- it sounds like this is finding and returning something but it's actually clicking a setting?customColor- from the naming it's not clear that the intention is to input a custom colourcolorTestId/imageUploadId- again, nothing here suggests that this is causing clicks, the naming suggests you're just providing some information
Maybe it would be better to have separate functions for specific actions?
utils/card-settings.js
selectNamedColor(testid, 'White')inputCustomColor(testid, '#ff0000')uploadImage(testid, filename)dragAndDropImage(testid, filename)
| const picker = page.locator(`[data-testid="color-picker-toggle"]`); | ||
| await picker.click(); | ||
| const colorInput = page.locator(`input[aria-label="Color value"]`); | ||
| await colorInput.click({clickCount: 3}); |
There was a problem hiding this comment.
Why does this need three clicks?
Code comments are really helpful for anything out of the ordinary like this to explain the why
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
145-148: Remove commented-out code.The commented-out
stopPropagationfunction is no longer used and a new implementation is provided later in the file. Remove this code to improve readability.- // const stopPropagation = useCallback((e) => { - // e.stopPropagation(); - // e.preventDefault(); - // }, []); -
214-220: Extract duplicated color wheel gradient styles.The same gradient styling code is duplicated in two places. Extract this to a shared variable or utility function to improve maintainability.
+ // At the top of the file, add this constant + const COLOR_WHEEL_STYLE = { + background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', + WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + WebkitMaskComposite: 'xor', + mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + maskComposite: 'exclude' + }; // Then replace both instances with: - <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - WebkitMaskComposite: 'xor', - mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - maskComposite: 'exclude' - }} /> + <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={COLOR_WHEEL_STYLE} />Also applies to: 285-291
139-143: Consider simplifying state management.The component uses three separate state variables (
isOpen,showColorPicker, andshowChildren) to manage UI visibility. This could lead to complex state interactions and potential bugs.Consider using a single state variable with an enum-like approach or a reducer to manage the UI state more predictably:
// Option 1: Use a string state const [uiState, setUiState] = useState('closed'); // 'closed', 'showingPicker', 'showingChildren' // Option 2: Use useReducer for more complex state management const [state, dispatch] = useReducer( (state, action) => { switch (action.type) { case 'TOGGLE_OPEN': return { ...state, isOpen: !state.isOpen, showColorPicker: false, showChildren: false }; case 'TOGGLE_COLOR_PICKER': return { ...state, showColorPicker: !state.showColorPicker, showChildren: false }; case 'SHOW_CHILDREN': return { ...state, isOpen: true, showChildren: true, showColorPicker: false }; case 'CLOSE': return { isOpen: false, showColorPicker: false, showChildren: false }; default: return state; } }, { isOpen: false, showColorPicker: false, showChildren: false } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (6)
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (5)
18-18: Good addition of the ImageUploadSwatch component.This import enables the new background image toggle functionality, replacing the previous button implementation with a more intuitive swatch component.
210-214: Well-structured handler function.This is a good implementation that centralizes the background image click handling logic. It properly manages state by showing the background image, expanding the color picker, and ensuring the button color picker is collapsed to avoid UI conflicts.
411-415: Clean implementation of the ImageUploadSwatch component.Using the ImageUploadSwatch inside the color picker's customContent provides a more intuitive UI for background image selection. The props pass-through is well structured.
421-421: Improved state clarity for the color picker.Using 'image' as the value when a background image is shown (instead of an empty string) makes the component state more explicit and easier to understand.
444-469: Enhanced conditional rendering of MediaUploadSetting.The refined conditional rendering makes the UI logic clearer by only showing the upload setting when appropriate (non-split layout with background image enabled). The proper cleanup in the
onRemoveMediahandler ensures the text color is updated correctly when the background image is removed.packages/koenig-lexical/src/components/ui/ColorPicker.jsx (1)
263-267: Clarify intent with commented-out code.There's a commented-out line (
setIsOpen(false)) which suggests uncertain behavior. Either remove it completely or uncomment it if the popover should close when a swatch is selected.onSelect={(val) => { onSwatchChange(val); setShowColorPicker(false); - // setIsOpen(false); + // If popover should remain open after swatch selection, remove this line + // If popover should close after swatch selection, uncomment: + // setIsOpen(false); }}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
144-147: Remove commented-out code.Commented-out code should be removed as it adds noise without providing value. If this was kept for reference, consider documenting the reasoning in a comment or remove it completely.
- // const stopPropagation = useCallback((e) => { - // e.stopPropagation(); - // e.preventDefault(); - // }, []);
255-258: Remove commented-out code and ensure consistent behavior.There's commented-out code
// setIsOpen(false);which suggests an unresolved design decision. This could lead to different behavior between color swatch selection and color picker usage.onSwatchChange(val); setShowColorPicker(false); -// setIsOpen(false);Consider either removing the comment or implementing the behavior consistently across all color change interactions.
210-216: Consider extracting repeated gradient styles.The same complex gradient CSS is duplicated in two places (lines 210-216 and 276-282). This should be extracted into a shared style, constant, or utility function to maintain DRY principles.
Create a shared gradient style:
// At the top of the file const colorWheelGradientStyle = { background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', WebkitMaskComposite: 'xor', mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', maskComposite: 'exclude' }; // Then use it in both locations: <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={colorWheelGradientStyle} />Also applies to: 276-282
packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (5)
5-9: Add prop validation and default props.The component is missing prop validation and default values. Consider adding PropTypes or TypeScript types to ensure type safety, and provide default values for the props, especially for optional ones like
dataTestId.+import PropTypes from 'prop-types'; import ImgBgIcon from '../../assets/icons/kg-img-bg.svg?react'; import clsx from 'clsx'; import {Tooltip} from './Tooltip'; export const ImageUploadSwatch = ({ showBackgroundImage, onClickHandler, dataTestId }) => {And at the end of the file:
}; + +ImageUploadSwatch.propTypes = { + showBackgroundImage: PropTypes.bool, + onClickHandler: PropTypes.func.isRequired, + dataTestId: PropTypes.string +}; + +ImageUploadSwatch.defaultProps = { + showBackgroundImage: false, + dataTestId: 'image-upload-swatch' +};
11-20: Improve accessibility for the button element.While the button has a
titleattribute, consider enhancing accessibility by adding anaria-labelattribute to improve screen reader support.<button className={clsx( `group relative flex size-6 shrink-0 items-center justify-center rounded-full border border-grey-300 bg-grey-100 text-black`, showBackgroundImage && 'outline outline-2 outline-green' )} data-testid={dataTestId} title="Image" + aria-label="Toggle background image" type="button" onClick={onClickHandler} >
11-20: Consider adding error handling to the click handler.The
onClickhandler is directly passed from props without any error checking. Consider wrapping it in a try-catch block or adding a conditional check to prevent potential runtime errors.<button className={clsx( `group relative flex size-6 shrink-0 items-center justify-center rounded-full border border-grey-300 bg-grey-100 text-black`, showBackgroundImage && 'outline outline-2 outline-green' )} data-testid={dataTestId} title="Image" type="button" - onClick={onClickHandler} + onClick={(e) => { + if (typeof onClickHandler === 'function') { + onClickHandler(e); + } + }} >
13-14: Use Tailwind's built-in utility classes for consistency.The component uses a mix of Tailwind utility classes and custom CSS. For better maintainability and consistency, consider using Tailwind's standard utility classes where possible.
<button className={clsx( - `group relative flex size-6 shrink-0 items-center justify-center rounded-full border border-grey-300 bg-grey-100 text-black`, + 'group relative flex h-6 w-6 shrink-0 items-center justify-center rounded-full border border-grey-300 bg-grey-100 text-black', showBackgroundImage && 'outline outline-2 outline-green' )}
5-25: Add test coverage for the new component.As this is a new UI component that will be used in multiple places, consider adding comprehensive test coverage to ensure it behaves as expected.
Consider creating a test file at
packages/koenig-lexical/src/components/ui/__tests__/ImageUploadSwatch.test.jsxwith tests for:
- Rendering with default props
- Rendering with showBackgroundImage=true
- Handling click events correctly
- Proper tooltip display
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (7)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (6)
9-9: Function signature updated to support composition pattern.The
ColorPickercomponent now acceptschildrenas a prop, enabling more flexible usage patterns where additional UI elements can be included alongside the color picker.
97-97: Good implementation of children rendering.The
childrenprop is correctly rendered within the component layout, allowing for extensible UI patterns with the ColorPicker.
138-142: Enhanced component interface with proper state management.The
ColorIndicatorcomponent has been significantly improved with:
- Additional props for better control (
onChange,eyedropper,hasTransparentOption,children)- State variables for managing UI visibility (
isOpen,showColorPicker,showChildren)- A ref for tracking the popover element
This provides a more flexible and powerful color selection experience.
149-163: Well-implemented click outside handling.The document click handler and associated useEffect hook properly manage the closing behavior of the popover. The cleanup of event listeners in the effect's return function is correctly implemented, preventing memory leaks.
193-196: Consider handling color picker interactions more consistently.The function
handleColorPickerChangepasses the color change to the parent but intentionally doesn't close the popover. The comment indicates this is a specific design choice, but consider whether this creates an inconsistent user experience compared to other interaction patterns in the component.Is this behavior aligned with the design requirements? Make sure the UX flow is consistent with user expectations when interacting with color pickers in the UI.
226-297: Well-structured popover with conditional rendering.The popover implementation properly uses conditional rendering to show either the color picker or children content based on state. The use of refs and event stopping ensures that the popover doesn't close unexpectedly while interacting with its contents.
packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (1)
1-25: Good implementation of a reusable UI component.The ImageUploadSwatch component is well-structured and follows the component extraction pattern correctly, improving code reusability across the HeaderCard and SignupCard components as mentioned in the PR description.
| <ImgBgIcon className="size-[1.4rem]" /> | ||
| <Tooltip label='Image' /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure proper tooltip positioning and accessibility.
The Tooltip component should have appropriate positioning and accessibility attributes to ensure it works correctly for all users.
🏁 Script executed:
#!/bin/bash
# Check how the Tooltip component is implemented to ensure it has proper accessibility support
cat packages/koenig-lexical/src/components/ui/Tooltip.jsxLength of output: 672
Action Required: Enhance Tooltip Accessibility and Keyboard Support
The current implementation of the Tooltip component (in packages/koenig-lexical/src/components/ui/Tooltip.jsx) renders a div with absolute positioning that only becomes visible on hover. However, it lacks ARIA attributes (e.g., role="tooltip") and does not address keyboard accessibility (e.g., focus-triggered visibility), which are essential for proper accessibility support.
- Add ARIA Attributes: Update the Tooltip’s container to include
role="tooltip". If applicable, consider addingaria-describedbyon the related interactive element. - Improve Keyboard Accessibility: Ensure that the tooltip is also shown on keyboard focus, not solely on hover.
- Review Positioning: Double-check that the absolute positioning aligns correctly with its target element in all cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (1)
214-220: Consider extracting the repeated gradient style to a reusable functionThe gradient styling for the color picker button appears twice in the code with identical properties. This is a good candidate for extraction to reduce duplication.
+ const colorWheelGradientStyle = { + background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', + WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + WebkitMaskComposite: 'xor', + mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + maskComposite: 'exclude' + }; // Then in the JSX: - <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - WebkitMaskComposite: 'xor', - mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - maskComposite: 'exclude' - }} /> + <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={colorWheelGradientStyle} />Also applies to: 285-291
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/signup-card.test.js(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (23)
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (12)
3-3: Improved code organization with helper import.Good addition of the
cardBackgroundColorSettingshelper, which will enhance code reusability across multiple test files and reduce duplication.
202-210: Well-structured refactoring using helper function.The test has been properly refactored to use the new
cardBackgroundColorSettingshelper function instead of direct element interactions. This approach is more maintainable and consistent.
227-230: Good use of consistent pattern for color tests.Using the same helper function for both button and background color tests ensures consistency in testing approach.
253-254: Simplified background image toggle implementation.The refactoring successfully combines color settings and image toggle functionality in a more streamlined way.
284-313: Well-implemented test for image icon verification.This new test properly verifies the UI feedback when a background image is selected. The path validation approach ensures the correct SVG icon is displayed.
325-326: Consistent implementation across test cases.Good consistent use of the helper function for handling the background image toggle.
337-338: Clean replacement of direct element selection.Properly commented out the old approach and replaced with the helper function. Good practice to keep the commented code for context during transition.
346-346: Seamless integration with existing test flow.The helper function integrates well with the existing test flow for switching back to image mode.
354-354: Consistent pattern for color picker interaction.Using the helper for color picker interaction maintains consistency across the test file.
369-370: Good replacement of direct element selection.Similar to other instances, proper replacement of direct element selection with the helper function.
379-381: Well-structured sequence for image upload interaction.The multi-step process for toggling to image mode and clicking the upload placeholder is clearly implemented.
390-391: Improved settings panel interaction.Good addition of explicit settings panel clicks to ensure proper UI state before and after operations.
Also applies to: 401-403
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (11)
10-10: New children prop enhances component flexibilityThe ColorPicker now accepts a children prop, which allows for custom content rendering alongside the color picker. This is a good improvement for component composability.
Also applies to: 98-98
139-143: Component API expansion improves functionalityThe ColorIndicator signature has been expanded with several new props that enhance its functionality:
onChange- Direct color change handlereyedropper- Controls eyedropper tool visibilityhasTransparentOption- Adds transparency optionchildren- Allows custom contentNew state variables properly manage the component's UI states.
150-164: Good implementation of click-outside detectionThe document click handler correctly detects clicks outside the component and closes the popover accordingly. The useEffect properly adds and removes event listeners based on the open state, preventing memory leaks.
166-169: Ensure stopPropagation doesn't interfere with parent componentsThe stopPropagation function prevents event bubbling, which is necessary for the color picker UI but could potentially interfere with parent components' event handling.
Test this component within various parent containers to ensure the event handling doesn't break expected behaviors in the parent components.
185-187: Proper handling of image background typeThe addition of the 'image' value type allows for image backgrounds to be properly represented in the color selection UI, which aligns with the PR objectives of improving the image-background selector.
197-200: ColorPicker change handler maintains open stateThe handleColorPickerChange function correctly updates the color without closing the popover, providing a better user experience when making color adjustments.
227-230: Clean implementation of image background indicatorThe conditional rendering of the ImgBgIcon when value is 'image' provides a clear visual indication of image backgrounds, which aligns with the PR objective of improving the image-background selector.
246-253: Well-structured conditional rendering of ColorPickerThe ColorPicker is conditionally rendered when showColorPicker is true, and all the necessary props are correctly passed down, including the newly added props from the signature change.
266-266: Consider if setIsOpen should be called when selecting a swatchCurrently, the comment at line 266 shows that setIsOpen(false) was intentionally removed. This means the color picker remains open after selecting a swatch, which might not be the expected behavior in all cases.
Confirm whether this is the intended behavior - keeping the popover open after swatch selection. If the intention is to close it, uncomment the setIsOpen(false) line.
254-254: Good pattern for rendering conditional childrenThe component correctly renders children conditionally based on the showChildren state, which aligns with the component API changes and provides flexibility for the parent components.
239-241: Dynamic width adjustment based on contentThe popover width dynamically adjusts using Tailwind's transition utilities when either the color picker or children are shown. This creates a smooth visual experience when toggling between different states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
399-402: Inconsistent default prop typesThere's an inconsistency in how default props are defined:
- Line 399:
imageDragHandler: () => {},(correct format)- Line 401:
toggleVisibility: PropTypes.func(should be a function, not a PropTypes definition)imageDragHandler: () => {}, - visibilityOptions: PropTypes.object, - toggleVisibility: PropTypes.func + visibilityOptions: {}, + toggleVisibility: () => {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(12 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(5 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (20)
packages/koenig-lexical/test/e2e/cards/header-card.test.js (12)
3-3: New helper import for consistent background color settingsThe import of
cardBackgroundColorSettingsfrom the background color helper is a good addition that will help standardize the way color settings are managed across tests.
190-191: Update to size selector class name assertionsThe test has been updated to check for
shadow-xsclass instead of the previousbg-grey-150, which aligns with the UI component changes mentioned in the PR objectives.
199-199: Consistent class name updates for size indicatorsSimilar updates to the class name checks for medium and large size buttons, ensuring consistent test assertions across all size options.
Also applies to: 210-210
226-226: Improved color selection workflow using helper functionReplaced direct button clicks with the
cardBackgroundColorSettingshelper function, which improves test readability and consistency across color change tests.Also applies to: 232-232, 238-238
251-253: Enhanced background image selection approachThe test now uses the standardized
cardBackgroundColorSettingshelper for initial setup, then proceeds with image selection via the UI, providing a more consistent testing approach.
529-534: Refactored button color configurationReplaced multiple manual steps for color selection with a single helper function call, simplifying the test and making it more maintainable.
551-557: Streamlined background color settingSimilar refactoring to use the helper function for background color selection, replacing multiple imperative steps with a declarative helper call.
578-580: Simplified image upload workflowImproved the image upload testing workflow by using the helper function for initial setup, making the test more maintainable and consistent with other tests.
589-591: Standardized color swatch selectionUpdated to use the
cardBackgroundColorSettingshelper with appropriate parameters for selecting a color by title, enhancing test consistency.
596-599: Consistent approach for switching between background typesImproved the way tests handle switching back to image background using the helper function, maintaining consistency with the rest of the test file.
606-607: Simplified color picker invocationUsing the helper function to open the color picker with a specific color, which standardizes the approach across all tests.
613-638: Added test for image icon visibilityThis new test verifies that when a background image is selected, the appropriate icon appears in the UI, which improves test coverage for the header card's visual feedback.
The test checks for the presence of a specific SVG path element, ensuring that the correct icon is displayed when a background image is set. This is a good addition to validate the visual feedback of the UI.
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (8)
24-26: Improvement: New color options addedThe addition of pink and purple colors to the CALLTOACTION_COLORS constant enhances the customization options available to users, which aligns with the PR objectives.
48-68: Color intensity adjustment for better visibilityThe background opacity for colors has been increased from /15 to /20, improving visibility while maintaining the subtle nature of the background colors.
69-79: New color options added to pickerThe pink and purple options have been correctly implemented in the color picker, matching the new CALLTOACTION_COLORS additions.
147-153: Improved testing capability with dataTestIdThe addition of a dataTestId for the background color picker will help with automated testing, making the tests more stable and less prone to breakage when the UI structure changes.
165-174: Button-style image upload implementationThe MediaUploadSetting has been enhanced with more descriptive text and support for drag-and-drop functionality, improving the user experience.
354-354: PropTypes updated to match new colorsThe PropTypes validation has been properly updated to include the new pink and purple color options.
375-377: New prop types for enhanced functionalityNew prop types have been added to support image drag-and-drop functionality and visibility toggling.
390-390:❓ Verification inconclusive
Behavioral change: Button shown by default
The default value for
showButtonhas been changed from false to true, which is a significant behavioral change. Ensure this is intentional and that all tests and implementations account for this change.
🏁 Script executed:
#!/bin/bash # Check whether other components expect the old default behavior rg "showButton={false}" --type js -g "*.jsx" rg "<CallToActionCard" -A 10 -B 5 --type js -g "*.jsx" | grep -v "showButton"Length of output: 3706
Behavioral Change: Default Button Visibility
The
CallToActionCardcomponent now setsshowButtontotrueby default instead offalse. Our review search (adjusting for the regex issue with curly braces) did not reveal any consumers explicitly relying on a default value offalsein either the story files or the parent component (CallToActionNodeComponent). However, please confirm that:
- This change was intentional.
- All tests—especially those related to button visibility—are updated accordingly.
- Downstream components correctly handle the button being shown by default.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (1)
387-388: Consider cleaning up unnecessary commented lines and whitespaceThe file contains several commented-out code lines and extra whitespace that could be removed for cleaner code.
- // data-testid="settings-panel" - - await page.locator('[data-testid="settings-panel"]').click(); // click here to close the colour swatchAlso applies to: 398-400
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
145-148: Remove commented code.The commented implementation of
stopPropagationis unnecessary as there's an active implementation at lines 166-169. Remove this dead code to improve readability.- // const stopPropagation = useCallback((e) => { - // e.stopPropagation(); - // e.preventDefault(); - // }, []);
264-267: Commented code in onSelect callback.There's a commented line that would close the popover when a swatch is selected. Consider removing this if the intended behavior is to keep the popover open after swatch selection.
onSelect={(val) => { onSwatchChange(val); setShowColorPicker(false); - // setIsOpen(false); }}
202-307: Consider extracting complex UI elements into separate components.The ColorIndicator component has grown significantly in complexity, with multiple nested conditional renderings and state management. Consider breaking this into smaller, focused components to improve maintainability and readability.
For example, you could extract:
- A ColorButton component for the main trigger button
- A ColorPopover component for the popover content
- A SwatchList component for the color swatches
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(9 hunks)packages/koenig-lexical/test/e2e/cards/signup-card.test.js(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (14)
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (4)
3-3: Great addition of the background color helper!Adding the
cardBackgroundColorSettingsutility import improves code maintainability by centralizing background color and image handling logic across multiple card tests.
206-209: Good refactoring to use the helper functionReplacing direct color picker interactions with the
cardBackgroundColorSettingshelper function improves readability and maintainability.
284-310: Excellent new test for image icon visibilityThis new test properly verifies that the UI provides clear visual feedback when a background image is selected, ensuring users can easily distinguish between color and image backgrounds.
334-335: Clean replacement of direct button interactionsReplacing the commented-out direct button click with the helper function is a good approach that standardizes interaction patterns across tests.
packages/koenig-lexical/test/e2e/cards/header-card.test.js (5)
3-3: Great addition of the background color helper!Adding the
cardBackgroundColorSettingsutility import improves test consistency across different card components.
190-192: Updated class assertions to match new UI component stylesThe test has been properly updated to check for
shadow-xsclass instead ofbg-grey-150, reflecting changes in how selection is visually indicated in the UI.
226-238: Good refactoring to use the helper functionReplacing direct color picker interactions with the
cardBackgroundColorSettingshelper function standardizes testing approach across components.
613-635: Excellent new test for image icon visibilityThis new test properly verifies that the UI provides clear visual feedback when a background image is selected, consistent with the same test added to signup-card.
529-534: Clean code refactoringReplacing multiple direct interactions with a single helper function call improves code readability and maintainability.
- // await page.click('[data-testid="header-button-color"] [aria-label="Pick color"]'); - - // await page.fill('[data-testid="header-button-color"] input', ''); - // await page.keyboard.type('ff0000'); - + await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'header-button-color', customColor: 'ff0000'});packages/koenig-lexical/src/components/ui/ColorPicker.jsx (5)
150-164: LGTM - Good implementation of click-outside handling.The document click listener implementation correctly handles closing the popover when clicking outside and properly cleans up event listeners to prevent memory leaks.
197-200: Good handling of color picker changes without closing the popover.The implementation correctly updates the color without closing the popover, enhancing the user experience.
10-10: Excellent API extension with optional children prop.The addition of the
childrenprop to both ColorPicker and ColorIndicator makes these components more flexible and reusable. This follows good React composition patterns by allowing consumers to extend the component functionality.Also applies to: 139-139
235-245: Good implementation of popover positioning and event handling.The popover implementation uses appropriate z-index, positioning, and transition effects. The event handling correctly stops propagation to prevent the popover from closing when interacting with its contents.
184-191:❓ Verification inconclusive
Verify image background behavior is consistent.
The new handling for
value === 'image'sets the background to transparent but uses a specific icon. Ensure this is consistent with how image backgrounds are represented in other components.
🏁 Script executed:
#!/bin/bash # Check how image backgrounds are represented in other components rg -A 3 -B 3 "value === 'image'" --type jsxLength of output: 79
Manual Verification Required: Image Background Behavior Consistency
The initial search encountered an error due to unrecognized file types for JSX. Please run the revised command above to verify how image backgrounds (i.e. instances where
value === 'image') are implemented in other components. This will ensure that setting the background to transparent with a specific icon inColorPicker.jsxis consistent with the rest of the codebase.
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run search for handling of image backgrounds across JS and JSX files rg -g "*.js" -g "*.jsx" -A 3 -B 3 "value === 'image'"Length of output: 1556
Action: Validate Image Background Handling Consistency
The current implementation in
ColorPicker.jsx(lines 184–191) sets the background color to transparent and uses<ImgBgIcon />forvalue === 'image'. Our repository-wide search shows that this handling is isolated to this component. Please manually verify that this approach is indeed consistent with the intended behavior for image backgrounds. In particular:
- Confirm that using a transparent background along with a specific swatch (using
swatches.find(swatch => swatch.image)?.title) and rendering<ImgBgIcon />aligns with how image backgrounds should be represented.- Double-check if any other components are expected to implement similar behavior and ensure consistency across the board.
- `defaultProps` is deprecated in favor of native destructing assignment defaults in props object - fixed `visibilityOptions` type - fixed `imageDragHandler` type - added new `selectedName` values of `'minimal'` and `'immersive'` to ButtonGroups.selectedName oneOf array - Fixed error from incorrect data-testId casing
- large parts of the component were repeated in order to surround varying contents - extracted contents to separate children to make behaviour clearer - improved classes assignment to remove duplication of the button's class list
- match node/component naming
- in some cases the hook's callbacks would be called when the ref'd element had already been removed, added guard to avoid trying to set style in that case
- interacting with other inputs or parts of the page did not close the color options popover as expected - added an effect to add a window-level event capture in order to close the popover on any mousedown - does not prevent events on the clicked item from firing as it could lead to the UI feeling unresponsive
- extracted click-outside behaviour from `ColorOptionsButtons` to custom `useClickOutside` hook - replaced click-outside behaviour in `ColorPicker` with `useClickOutside`
- the default color of the button is the accent color (with correct fallbacks) so there's no need to override it when accent is selected
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (1)
185-215: Enhance the color indicator button with proper accessibility attributes.The button implementation is visually appealing but needs accessibility improvements.
- <button - className={`relative size-6 cursor-pointer rounded-full ${value ? 'p-[2px]' : 'border border-grey-200 dark:border-grey-800'}`} - type="button" - onClick={() => { - setIsOpen(!isOpen); - setShowColorPicker(false); - setShowChildren(false); - }} - > + <button + className={`relative size-6 cursor-pointer rounded-full ${value ? 'p-[2px]' : 'border border-grey-200 dark:border-grey-800'}`} + type="button" + aria-haspopup="dialog" + aria-expanded={isOpen} + aria-label="Select color" + onClick={() => { + setIsOpen(!isOpen); + setShowColorPicker(false); + setShowChildren(false); + }} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/koenig-lexical/src/components/ui/Button.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.jsx(3 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx(4 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(11 hunks)packages/koenig-lexical/src/hooks/useClickOutside.js(1 hunks)packages/koenig-lexical/src/hooks/useMovable.js(1 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/koenig-lexical/src/components/ui/ButtonGroup.jsx
- packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (32)
packages/koenig-lexical/src/hooks/useMovable.js (2)
127-130: Good defensive programming with null checkAdding the null check for
ref.currentbefore accessing its style property is a good defensive programming practice. This prevents potential TypeError exceptions that could occur if the component using this hook unmounts during a drag operation or if the ref hasn't been initialized yet.
134-137: Good addition of null checkSimilarly, this null check for
ref.currentin theenablePointerEventsfunction prevents potential runtime errors. These defensive checks improve the robustness of the hook and ensure it gracefully handles edge cases where the referenced element may no longer exist.packages/koenig-lexical/src/components/ui/Button.jsx (1)
5-19: Good improvement to the component's default props implementation!Moving default values directly into the function parameters is a cleaner, more modern React approach compared to using the separate
defaultPropsobject. This change maintains the same functionality while making the code more maintainable and aligned with current React best practices.packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (12)
2-3: Good addition of utility imports.The new imports for
createDataTransferandcardBackgroundColorSettingsare well-placed to support the testing improvements in this file.
122-129: Test name properly reflects the new default behavior.The test has been renamed from toggling to checking default visibility, which aligns with the component's updated behavior where buttons and settings are visible by default.
131-141: Test logic improved to check both states.The test now properly verifies both the initial visibility and the toggled state of the button, providing better coverage of the component's behavior.
218-219: Good refactoring to use the helper function.Replacing the direct click with the
cardBackgroundColorSettingshelper improves maintainability and consistency across tests.
228-229: Consistent use of helper function for color settings.Using the same helper function for all color operations ensures consistency and reduces code duplication.
236-237: Helper function properly handles custom color selection.The helper function is correctly used to set a custom color, which streamlines the testing approach.
244-246: Simplified color testing with helper function.The helper function is used consistently here for setting white color, making the test more readable and maintainable.
315-318: Added new color options.Pink and purple background colors have been added to the CTA card options, expanding the available customization options.
323-324: Updated assertion to include new colors.The assertion has been properly updated to include the new pink and purple background colors in the regex pattern.
325-327: Updated selectors for background color selection.The selector approach has been updated to use a more targeted method for interacting with color options.
331-345: Good addition of test for color popup behavior.This test improves coverage by ensuring that the background color popup closes when clicking outside of it, which is an important UX feature.
367-382: Excellent addition of drag-and-drop test.This test adds coverage for the drag-and-drop functionality for image uploads, using the new
createDataTransferutility function. It tests both the dragover state and the successful drop outcome.packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (7)
39-42: Similar conditional rendering issue in StandardContents.Like the
ButtonContentscomponent,StandardContentsreturns undefined whensize === 'xsmall' && hasErrors. This may lead to unexpected rendering behavior. Consider returning null explicitly for consistency.
20-30: Good component extraction with CardText.The extraction of
CardTextinto a separate component improves code modularity and reuse. The use ofclsxfor conditional class application based on the type prop is clean and maintainable.
91-95: Potential duplication of button styling code.This
buttonClassesdefinition partially overlaps with the inline version in theCardTextcomponent. To keep the codebase DRY, consider consolidating them.Consider extracting the common padding classes to a constant or consolidating the styling logic:
export const CardText = ({text, type}) => ( <span className={clsx( 'text-center font-sans text-sm font-semibold text-grey-800 transition-all group-hover:text-grey-800', - type === 'button' && 'px-3 py-1' + type === 'button' && BUTTON_PADDING_CLASSES )} data-kg-card-drag-text > {text} </span> ); +const BUTTON_PADDING_CLASSES = 'px-3 py-1'; const buttonClasses = clsx( 'group flex cursor-pointer select-none items-center justify-center', - type === 'button' && 'px-3 py-1', + type === 'button' && BUTTON_PADDING_CLASSES, type !== 'button' && (size === 'xsmall' ? 'p-4' : 'flex-col p-20') );
82-89: Well-structured container class definitions.The use of
clsxfor conditional class application is clean and maintainable. The separation of styling logic based ontype,size, andborderStylemakes the code more readable and easier to maintain.
129-132: Clean conditional rendering based on type.The conditional rendering based on the
typeprop is well-implemented, using the appropriate component for each case. This enhances the flexibility of the MediaPlaceholder component.
146-147: PropTypes properly updated for the new type prop.The PropTypes have been correctly updated to include validation for the new
typeprop, ensuring type safety.
134-135: Error messages rendering improved.The error messages are now rendered consistently with the chosen component type, which improves the user experience.
packages/koenig-lexical/src/hooks/useClickOutside.js (1)
1-18: Well-implemented custom hook for detecting outside clicks.This is a clean implementation of a reusable click outside detection hook. It follows React best practices with proper effect cleanup and dependency management.
A few minor considerations for future enhancements:
- Consider adding support for touch events (touchstart) for better mobile support
- You might want to add a null check before calling
containsonref.currentpackages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (4)
8-14: Good implementation of popover state management.The use of useState and useClickOutside hook provides a clean way to manage the popover visibility state.
17-36: Enhance accessibility for the color options button.The button implementation is good, but could benefit from improved accessibility attributes.
- <button - className={`relative size-6 cursor-pointer rounded-full ${selectedName ? 'p-[2px]' : 'border border-grey-200 dark:border-grey-800'}`} - data-testid="color-options-button" - type="button" - onClick={() => setIsOpen(!isOpen)} - > + <button + className={`relative size-6 cursor-pointer rounded-full ${selectedName ? 'p-[2px]' : 'border border-grey-200 dark:border-grey-800'}`} + data-testid="color-options-button" + type="button" + aria-haspopup="true" + aria-expanded={isOpen} + aria-label="Color options" + onClick={() => setIsOpen(!isOpen)} + >
39-67: Enhance accessibility for the color options popover.The color options popover needs proper ARIA attributes for better accessibility.
- {isOpen && ( - <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow" data-testid="color-options-popover"> + {isOpen && ( + <div + className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow" + data-testid="color-options-popover" + role="dialog" + aria-label="Color options" + >- <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"> + <ul + className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white" + role="listbox" + aria-label="Available colors" + >
77-77: LGTM - Consistent styling for list items.The styling update for list items looks good.
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (5)
11-11: Good API enhancement with children prop support.Adding the children prop provides more flexibility for component composition.
81-81: Improved flexibility with children prop being rendered.The ColorPicker component now properly renders children components, enhancing its composability.
Also applies to: 99-99
140-160: Improved state management for ColorIndicator component.The component now has better state management with clear separation of concerns for different UI states (isOpen, showColorPicker, showChildren).
Consider adding keyboard navigation support for better accessibility:
+ const handleKeyDown = useCallback((e) => { + if (e.key === 'Escape' && isOpen) { + setIsOpen(false); + } + }, [isOpen]); + + useEffect(() => { + document.addEventListener('keydown', handleKeyDown); + return () => document.removeEventListener('keydown', handleKeyDown); + }, [handleKeyDown]);
179-182: Good UX improvement for color picker.Keeping the popover open when using the color picker is a good UX decision.
217-287: Improved color selection UI with support for custom content.The popover implementation is much improved, with better support for color swatches, custom content, and color picker toggling.
Add role attributes to improve accessibility:
- <div - ref={popoverRef} - className={clsx( - 'absolute -right-3 bottom-full z-10 mb-2 flex flex-col gap-3 rounded-lg bg-white p-3 shadow transition-[width] duration-200 ease-in-out dark:bg-grey-950', - (showColorPicker || showChildren) && 'min-w-[296px]' - )} - onClick={stopPropagation} - onMouseDown={stopPropagation} - onTouchStart={stopPropagation} - > + <div + ref={popoverRef} + role="dialog" + aria-label="Color selection" + className={clsx( + 'absolute -right-3 bottom-full z-10 mb-2 flex flex-col gap-3 rounded-lg bg-white p-3 shadow transition-[width] duration-200 ease-in-out dark:bg-grey-950', + (showColorPicker || showChildren) && 'min-w-[296px]' + )} + onClick={stopPropagation} + onMouseDown={stopPropagation} + onTouchStart={stopPropagation} + >
| const ButtonContents = ({desc, hasErrors}) => { | ||
| if (hasErrors) { | ||
| return; | ||
| } | ||
| return <p className="!font-sans !text-[1.3rem] !font-medium text-grey-900">{desc}</p>; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check conditional rendering in ButtonContents component.
The ButtonContents component returns undefined when hasErrors is true, which could lead to unexpected rendering behavior. Consider returning null explicitly or implementing an alternative error state UI for better user experience.
🏁 Script executed:
#!/bin/bash
# Check how this component handles error states in actual usage
rg -A 5 "ButtonContents" --type jsLength of output: 1283
Action Required: Improve error state rendering in ButtonContents
The current implementation returns undefined when hasErrors is true, which can lead to unexpected rendering behavior. Please update this conditional so that it explicitly returns null (or an alternative error state UI) to ensure React handles the component correctly.
-
Location:
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx, Lines: 32-37 -
Suggested Change:
const ButtonContents = ({desc, hasErrors}) => { if (hasErrors) { return null; // Explicitly return null to prevent rendering issues } return <p className="!font-sans !text-[1.3rem] !font-medium text-grey-900">{desc}</p>; };
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (1)
82-89: Consider refactoring complex container class conditions for readability.While the conditional classes work correctly, the nested conditions based on type, size, and borderStyle make the code harder to follow. Consider breaking these into smaller, more focused helper functions or variables.
- const containerClasses = clsx( - 'relative flex h-full items-center justify-center', - type === 'button' ? 'rounded-lg bg-grey-100' : 'border bg-grey-50', - size === 'xsmall' && type !== 'button' && 'before:pb-[12.5%] dark:bg-grey-900', - size !== 'xsmall' && type !== 'button' && 'before:pb-[62.5%] dark:bg-grey-950', - borderStyle === 'rounded' && type !== 'button' && 'rounded-lg border-grey/20 dark:border-transparent', - borderStyle !== 'rounded' && type !== 'button' && 'border-grey/20 dark:border-grey/10' - ); + // Base container classes common to all types + const baseContainerClasses = 'relative flex h-full items-center justify-center'; + + // Background and border classes based on type + const typeClasses = type === 'button' + ? 'rounded-lg bg-grey-100' + : 'border bg-grey-50'; + + // Classes specific to non-button types + const nonButtonClasses = type !== 'button' ? { + // Size-based classes + sizeClasses: size === 'xsmall' + ? 'before:pb-[12.5%] dark:bg-grey-900' + : 'before:pb-[62.5%] dark:bg-grey-950', + + // Border style classes + borderClasses: borderStyle === 'rounded' + ? 'rounded-lg border-grey/20 dark:border-transparent' + : 'border-grey/20 dark:border-grey/10' + } : {}; + + const containerClasses = clsx( + baseContainerClasses, + typeClasses, + type !== 'button' && nonButtonClasses.sizeClasses, + type !== 'button' && nonButtonClasses.borderClasses + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (11)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (11)
9-9: Good addition of the clsx library.The import of clsx enables more readable conditional class handling throughout the component, which is appropriate given the increased complexity of class management introduced in this PR.
20-30: Well-structured CardText component with improved flexibility.The updated CardText component nicely handles conditional styling based on the new
typeprop, with appropriate visual treatment for buttons. The change fromfont-boldtofont-semiboldalso aligns with modern UI design trends.
32-37: Fixed error state handling in ButtonContents.The component now explicitly returns
nullwhenhasErrorsis true, addressing the previous review feedback and ensuring proper React rendering behavior.
39-65: Clean implementation of StandardContents with appropriate conditional styling.The component effectively handles different size variations and error states. The separation of icon and description class logic improves readability.
72-73: Good addition of type prop and explicit default value.Adding the
typeprop enables the button-style image upload placeholder mentioned in the PR objectives. Setting an explicit default forborderStyleimproves code clarity.
91-95: Good refactoring of button styles.This refactoring addresses the previous review comment about duplicated button styling code by consolidating styles into a single
buttonClassesvariable.
97-100: Clean error styling implementation.The error styling is well organized and properly adapts based on size variations.
102-110: Good error message rendering approach.Using a map to render error messages is an appropriate pattern, and the key usage for list items is correctly implemented.
115-116: Appropriate class update and props spreading.The updated class for the container is more specific and doesn't interfere with prose styles, and the props are correctly spread.
119-137: Clean implementation of conditional rendering based on type.The component now correctly handles different visual presentations based on type, with appropriate handling of drag states and errors.
146-146: PropTypes correctly updated for new type prop.The PropTypes definition has been correctly updated to include validation for the new
typeprop with appropriate allowed values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
128-130: Consider adding explicit error handlingThe handleImageDrop function is a simple wrapper that awaits handleImageChange, but it doesn't have its own error handling. While handleImageChange has a try/finally block, errors aren't explicitly caught.
async function handleImageDrop(files) { - await handleImageChange(files); + try { + await handleImageChange(files); + } catch (error) { + console.error('Error handling image drop:', error); + // Consider showing user feedback for the error + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kg-default-nodes/test/nodes/call-to-action.test.js(3 hunks)packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (3)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (3)
4-4: Good addition of drag and drop functionalityAdding the useFileDragAndDrop hook enhances the component's interactivity by supporting image file drag and drop, which aligns with the PR objectives of improving UI components.
35-35: Good implementation of the drag handlerThe imageDragHandler is properly configured with the handleImageDrop callback, following the component's existing patterns for event handling.
149-149: Good prop passing to the CallToActionCardCorrectly passing the imageDragHandler to the CallToActionCard component, enabling the drag and drop functionality in the UI.
- removes changes from non-beta cards/components - updated beta CTA card to use beta UI components - allows other cards to be switched to new UI later behind a flag for safer releases
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (2)
49-51: Incomplete PropTypes definition.The PropTypes only include validation for selectedName, but other props like buttons and onClick are missing from the definition.
Consider adding the missing prop validations:
ButtonGroupBeta.propTypes = { selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + buttons: PropTypes.arrayOf(PropTypes.shape({ + label: PropTypes.string, + name: PropTypes.string.isRequired, + Icon: PropTypes.elementType, + dataTestId: PropTypes.string + })), + onClick: PropTypes.func };Also, consider adding PropTypes for the IconButton component for completeness:
IconButton.propTypes = { dataTestId: PropTypes.string, onClick: PropTypes.func, label: PropTypes.string, name: PropTypes.string.isRequired, selectedName: PropTypes.string, Icon: PropTypes.elementType };
35-37: Long inline className with multiple conditional expressions.The lengthy inline className with multiple conditions could be harder to maintain as the component evolves.
Consider using a more structured approach with clsx or classnames library:
-className={`group relative flex h-7 w-8 cursor-pointer items-center justify-center rounded-lg text-black dark:text-white dark:hover:bg-grey-900 ${isActive ? 'border border-grey-300 bg-white shadow-xs dark:bg-grey-900' : '' } ${Icon ? '' : 'text-[1.3rem] font-bold'}`} +className={clsx( + 'group relative flex h-7 w-8 cursor-pointer items-center justify-center rounded-lg text-black dark:text-white dark:hover:bg-grey-900', + isActive && 'border border-grey-300 bg-white shadow-xs dark:bg-grey-900', + !Icon && 'text-[1.3rem] font-bold' +)}packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.jsx (2)
142-148: Incomplete PropTypes definition.The PropTypes only include validation for some props but miss several others that are used in the component.
Consider extending the PropTypes to include all props:
MediaPlaceholderBeta.propTypes = { icon: PropTypes.oneOf(['image', 'gallery', 'video', 'audio', 'file', 'product']), desc: PropTypes.string, size: PropTypes.oneOf(['xsmall', 'small', 'medium', 'large']), type: PropTypes.oneOf(['image', 'button']), - borderStyle: PropTypes.oneOf(['squared', 'rounded']) + borderStyle: PropTypes.oneOf(['squared', 'rounded']), + filePicker: PropTypes.func, + isDraggedOver: PropTypes.bool, + errors: PropTypes.arrayOf(PropTypes.shape({ + message: PropTypes.string.isRequired + })), + placeholderRef: PropTypes.oneOfType([ + PropTypes.func, + PropTypes.shape({ current: PropTypes.instanceOf(Element) }) + ]), + dataTestId: PropTypes.string, + errorDataTestId: PropTypes.string, + multiple: PropTypes.bool };
36-36: Usage of !important CSS rules in multiple places.The use of !important flags in tailwind classes (e.g.,
!font-sans,!text-sm,!mt-0) might indicate CSS specificity issues that could be resolved more elegantly.Consider reviewing the CSS structure to eliminate the need for !important flags. This could involve:
- Adjusting the specificity of your base styles
- Using more specific selectors in the component
- Organizing CSS layers differently if using a layered approach
This would make the styles more maintainable and less prone to unexpected overrides.
Also applies to: 56-58
packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsx (1)
24-31: Refactor inline swirl gradient into a dedicated CSS class.Using inline styles for complex backgrounds can make the code harder to maintain and read. Consider moving the swirl gradient into a well-named CSS class to improve clarity.
- <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(...)', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - WebkitMaskComposite: 'xor', - mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - maskComposite: 'exclude' - }} /> + <div className="absolute inset-0 rounded-full bg-clip-content p-[3px] swirl-gradient" />packages/koenig-lexical/src/components/ui/ColorPickerBeta.jsx (2)
49-63: Ensure graceful fallback for EyeDropper API.The EyeDropper API is not supported on all browsers yet. Consider adding a fallback path or a feature detect to avoid runtime errors for unsupported browsers.
140-290: Separate complex components into individual files for better maintainability.ColorIndicatorBeta, ColorPickerBeta, and ColorSwatch are all non-trivial. Consider moving each into its own file to adhere to single-responsibility principles and improve readability.
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (1)
118-139: Consider adding default props or runtime checks for optional callbacks.Not all callbacks (e.g.,
onRemoveMedia) may be provided consistently, and the component could break if a parent omits them. Default props help prevent errors and make usage clearer.packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (4)
10-10: Confirm usage of both Beta and non-Beta imports.You’re importing
ButtonGroupSettingBeta,ColorOptionSettingBeta, etc. If the old versions are no longer needed, consider removing them to avoid confusion and duplication within the codebase.
140-153: Combine or deprecate older settings in the future.
ButtonGroupSettingBetaandColorOptionSettingBetaare introduced here while old versions still exist in the codebase. If the plan is to migrate everything to Beta, consider marking the older versions as deprecated to maintain clarity.
187-220: Assess performance when toggling the color picker.
ColorPickerSettingBetamight trigger extra renders if it’s re-initialized often. If performance becomes an issue, consider memoizing the component or controlling the expand/collapse state with a stable callback.
108-110: EnsuretoggleVisibilityandimageDragHandlerroles are clearly documented.These new props appear in the prop types; ensure that their usage patterns are communicated in a README or docstring to prevent confusion among contributors.
Also applies to: 374-377
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (5)
6-15: Eliminate duplicates if Beta versions fully replace old imports.You’re importing both
ButtonGroupandButtonGroupBeta,MediaUploaderandMediaUploaderBeta, etc. If the old components are no longer in use, consider removing them to streamline maintenance.
238-248: Plan to unify or retire legacy button groups.
ButtonGroupSettingBetausesButtonGroupBeta. If “Beta” is production-ready, unify naming to reduce confusion between multiple button group components.
262-272: Refactor repeated logic in color option settings.
ColorOptionSettingandColorOptionSettingBetashare similar structures. Centralizing shared code or converting to a single approach can reinforce consistency.
317-344: Confirm the child slot usage inColorPickerSettingBeta.Allowing
childrento be passed is flexible, but confirm that future usage is well-defined (e.g., special toolbar items). Provide thorough documentation or usage examples if needed.
346-378: Review partial duplication betweenMediaUploadSettingvs.MediaUploadSettingBeta.The updated signature and usage of Beta might eventually replace the old function. Maintaining both can lead to drift. Evaluate merging them or systematically deprecating the older setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ButtonGroupBeta.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorPickerBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorPickerBeta.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(13 hunks)packages/koenig-lexical/test/e2e/cards/gallery-card.test.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.stories.jsx
[error] 84-84: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (17)
packages/koenig-lexical/test/e2e/cards/gallery-card.test.js (1)
510-511: Nice improvement to the gallery image export test!Good addition of the progress bar visibility check before verifying the image count. This ensures that all uploads are complete and the UI is in a stable state before making assertions, which helps prevent flaky tests.
Also, using
getByTestId()instead oflocator()is more idiomatic for Playwright tests and improves readability.packages/koenig-lexical/src/components/ui/ButtonGroupBeta.stories.jsx (2)
1-20: Well-structured Storybook setup with clear component documentation.The story configuration is well-organized with appropriate metadata, and the component's selectable options are properly defined in the argTypes. This makes it easy for developers to experiment with different button group states.
28-48: Good example implementation of the CardWidth story.The CardWidth story provides a practical example with meaningful labels and corresponding icons, making it clear how the ButtonGroupBeta component should be used to create image width selectors.
packages/koenig-lexical/src/components/ui/ColorPickerBeta.stories.jsx (2)
1-16: Well-configured story with appropriate control options.The story setup is clean and provides a good selection of color options in the control. The status parameter correctly indicates this is a UI-ready component.
18-33: Good implementation with appropriate container constraints.The Template wraps the ColorPickerBeta in a fixed-width container (240px), which is a good practice for components that need specific dimensional constraints to display properly in Storybook. The Default story provides a good variety of swatch types (brand color, standard hex, and transparent).
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (2)
7-25: Clean implementation of the ButtonGroupBeta component.The component effectively maps through the provided buttons array and renders IconButton components with the appropriate props.
27-47: Well-implemented IconButton with appropriate focus handling.The component nicely integrates with the usePreviousFocus hook and properly handles conditional rendering of icons and tooltips.
packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.jsx (3)
11-18: Well-organized placeholder icons mapping.The PLACEHOLDER_ICONS object provides a clean way to map media types to their corresponding icons, making the code more maintainable.
67-81: Comprehensive component props with good defaults.The component accepts a wide range of props with appropriate defaults, enhancing its flexibility and reusability across different contexts.
119-136: Event propagation in the button click handler.The current implementation doesn't prevent event propagation for the button click, which could be an issue if this component is used within other clickable elements.
Consider whether event propagation should be stopped to prevent unintended interactions:
<button className={buttonClasses} name="placeholder-button" type="button" - onClick={filePicker} + onClick={(e) => { + e.stopPropagation(); + filePicker(e); + }} >This change would be important if the PR objectives mention addressing event bubbling issues in image upload interactions, as stated in the PR summary.
packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsx (1)
72-93: Validation for keyboard accessibility.Although
usePreviousFocushelps maintain focus handling, ensure thorough keyboard/mouse testing to confirm that focusing and user navigation do not break with all possible interactions (e.g., repeated openings and closings of the popover).packages/koenig-lexical/src/components/ui/ColorPickerBeta.jsx (1)
81-103: Avoid unintentional popover closures in nested interactions.You’re stopping event propagation to keep the color picker open on interactions, which is fine, but watch out for side effects when nesting within other clickable components. Verify there's no unexpected behavior in layered popovers or modals.
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (2)
52-78: Handle edge cases for empty media states more thoroughly.When
isEmptyis true, the component only renders a placeholder. Consider clarifying or confirming:
- How errors are displayed if the user tries uploading invalid files.
- Whether any default alt or tooltip should be shown if accessibility is a concern.
80-103: Confirm the external image editor’s event shape.The edit button triggers an
onFileChangecallback with a custom event-like shape ({ target: { files: [editedImage] } }). Ensure that all consumers ofonFileChangecan handle this correctly.packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (2)
24-79: Validate the new color additions and updated alpha values.The introduction of
pinkandpurpleplus the switch frombg-color/10tobg-color/20is a meaningful design change. Confirm that these new backgrounds align with UI/UX guidelines and do not trigger contrast or accessibility issues.
163-177: Check for consistent behavior inMediaUploadSettingBeta.Replacing the older media upload setting with a “Beta” version can cause scattered references. Verify that all calls (including test files) now use this new setting consistently and that no references remain to the old component.
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
379-410: Ensure thorough test coverage forMediaUploadSettingBeta.It’s a new implementation referencing
MediaUploaderBeta. Confirm end-to-end functionality (drag and drop, display states, error handling, etc.).
| borderStyle: 'squared' | ||
| }; | ||
|
|
||
| export const Error = Template.bind({}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename the "Error" export to avoid shadowing the global Error object.
Using "Error" here is flagged as a lint warning. Rename this to avoid potential conflicts with the built-in Error.
-export const Error = Template.bind({});
+export const ErrorState = Template.bind({});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const Error = Template.bind({}); | |
| export const ErrorState = Template.bind({}); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| export function MediaUploaderBeta({ | ||
| className, | ||
| imgClassName, | ||
| src, | ||
| alt, | ||
| desc, | ||
| icon, | ||
| size, | ||
| type, | ||
| borderStyle = 'squared', | ||
| backgroundSize = 'cover', | ||
| mimeTypes, | ||
| onFileChange, | ||
| dragHandler, | ||
| isEditing = true, | ||
| isLoading, | ||
| isPinturaEnabled, | ||
| openImageEditor, | ||
| progress, | ||
| errors, | ||
| onRemoveMedia, | ||
| additionalActions, | ||
| setFileInputRef | ||
| }) { |
There was a problem hiding this comment.
Add missing prop type definitions for newly used props.
The component destructures props like isEditing, additionalActions, imgClassName, backgroundSize, isPinturaEnabled, and setFileInputRef, but they are not declared in the propTypes block. This can cause runtime inconsistencies and confusions.
You can fix it by adding these missing definitions in MediaUploaderBeta.propTypes. For example:
+ isEditing: PropTypes.bool,
+ additionalActions: PropTypes.node,
+ imgClassName: PropTypes.string,
+ backgroundSize: PropTypes.oneOf(['cover', 'contain']),
+ isPinturaEnabled: PropTypes.bool,
+ setFileInputRef: PropTypes.func,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function MediaUploaderBeta({ | |
| className, | |
| imgClassName, | |
| src, | |
| alt, | |
| desc, | |
| icon, | |
| size, | |
| type, | |
| borderStyle = 'squared', | |
| backgroundSize = 'cover', | |
| mimeTypes, | |
| onFileChange, | |
| dragHandler, | |
| isEditing = true, | |
| isLoading, | |
| isPinturaEnabled, | |
| openImageEditor, | |
| progress, | |
| errors, | |
| onRemoveMedia, | |
| additionalActions, | |
| setFileInputRef | |
| }) { | |
| import PropTypes from 'prop-types'; | |
| export function MediaUploaderBeta({ | |
| className, | |
| imgClassName, | |
| src, | |
| alt, | |
| desc, | |
| icon, | |
| size, | |
| type, | |
| borderStyle = 'squared', | |
| backgroundSize = 'cover', | |
| mimeTypes, | |
| onFileChange, | |
| dragHandler, | |
| isEditing = true, | |
| isLoading, | |
| isPinturaEnabled, | |
| openImageEditor, | |
| progress, | |
| errors, | |
| onRemoveMedia, | |
| additionalActions, | |
| setFileInputRef | |
| }) { | |
| // Component implementation | |
| } | |
| MediaUploaderBeta.propTypes = { | |
| // You may have existing propTypes here; add the missing ones: | |
| isEditing: PropTypes.bool, | |
| additionalActions: PropTypes.node, | |
| imgClassName: PropTypes.string, | |
| backgroundSize: PropTypes.oneOf(['cover', 'contain']), | |
| isPinturaEnabled: PropTypes.bool, | |
| setFileInputRef: PropTypes.func, | |
| // ... any other prop types | |
| }; |
Ref https://linear.app/ghost/issue/PLG-355/improve-cta-card-settings-panel
stopPropagationto Image Upload Form to prevent event bubbling issues.These updates enhance UI consistency, improve test reliability, and ensure smoother user interactions.