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 Possibly related PRs
Suggested reviewers
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.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.
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx
Outdated
Show resolved
Hide resolved
| @@ -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
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} + >
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.).
packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.stories.jsx
Outdated
Show resolved
Hide resolved
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx
Outdated
Show resolved
Hide resolved
No ref - There's no good way to describe the layout nuances in a single word, so tooltip is confusing rather than helpful - Added more descriptive aria labels to layout buttons
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
379-410: 🛠️ Refactor suggestionConsider reducing duplication between MediaUploadSetting and MediaUploadSettingBeta.
The MediaUploadSettingBeta component is nearly identical to MediaUploadSetting, with the only difference being the use of MediaUploaderBeta instead of MediaUploader. This duplication makes maintenance more difficult as changes need to be made in both places.
Consider creating a shared helper function or component to reduce duplication:
+ function createMediaUploadSetting(UploaderComponent) { + return function MediaUploadSettingImpl({className, label, hideLabel, onFileChange, isDraggedOver, placeholderRef, src, alt, isLoading, errors = [], progress, onRemoveMedia, icon, desc = '', size, type, stacked, borderStyle, mimeTypes, isPinturaEnabled, openImageEditor, setFileInputRef}) { + return ( + <div className={clsx(className, !stacked && 'flex justify-between gap-3')} data-testid="media-upload-setting"> + <div className={hideLabel ? 'sr-only' : 'mb-2 shrink-0 text-sm font-medium tracking-normal text-grey-900 dark:text-grey-400'}>{label}</div> + + <UploaderComponent + alt={alt} + borderStyle={borderStyle} + className={clsx( + stacked && 'h-32', + !stacked && src && 'h-[5.2rem]', + !stacked && type !== 'button' && !src && 'h-[5.2rem] w-[7.2rem]' + )} + desc={desc} + dragHandler={{isDraggedOver, setRef: placeholderRef}} + errors={errors} + icon={icon} + isLoading={isLoading} + isPinturaEnabled={isPinturaEnabled} + mimeTypes={mimeTypes} + openImageEditor={openImageEditor} + progress={progress} + setFileInputRef={setFileInputRef} + size={size} + src={src} + type={type} + onFileChange={onFileChange} + onRemoveMedia={onRemoveMedia} + /> + </div> + ); + }; + } + + export const MediaUploadSetting = createMediaUploadSetting(MediaUploader); + export const MediaUploadSettingBeta = createMediaUploadSetting(MediaUploaderBeta);This approach would make future maintenance easier as changes only need to be made in one place.
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (2)
7-27: Consider adding more prop validation for the buttons array.The component has good structure and functional implementation for rendering a group of buttons. However, the buttons array structure lacks PropTypes validation, which could help catch issues early during development.
+ ButtonGroupBeta.propTypes = { + buttons: PropTypes.arrayOf(PropTypes.shape({ + label: PropTypes.string, + name: PropTypes.string.isRequired, + Icon: PropTypes.elementType, + dataTestId: PropTypes.string, + ariaLabel: PropTypes.string + })), + onClick: PropTypes.func.isRequired, + selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + hasTooltip: PropTypes.bool + };
29-49: Consider improving code readability for conditional class assignments.The IconButton component correctly implements the button UI with appropriate behavior for active state, tooltip display, and click handling. However, the className assignment on line 38 is complex and could be improved for readability.
- 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' + )}Also, consider adding PropTypes for the IconButton component to ensure it receives properly typed props.
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
346-377: Enhanced mediaUploadSetting with improved conditional formatting.The conditional logic for className has been improved to handle different scenarios based on the new
typeparameter. The implementation is good, but consider adding a default value for thedescparameter to ensure consistent behavior.- export function MediaUploadSetting({className, label, hideLabel, onFileChange, isDraggedOver, placeholderRef, src, alt, isLoading, errors = [], progress, onRemoveMedia, icon, desc, size, type, stacked, borderStyle, mimeTypes, isPinturaEnabled, openImageEditor, setFileInputRef}) { + export function MediaUploadSetting({className, label, hideLabel, onFileChange, isDraggedOver, placeholderRef, src, alt, isLoading, errors = [], progress, onRemoveMedia, icon, desc = '', size, type, stacked, borderStyle, mimeTypes, isPinturaEnabled, openImageEditor, setFileInputRef}) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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 (5)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (2)
1-5: LGTM: Clean import structure.The imports are well-organized, importing only the necessary dependencies for the component's functionality.
51-54:❓ Verification inconclusive
Reconsider the restrictive selectedName validation.
The PropTypes definition for
selectedNamerestricts it to a predefined set of values, which may limit reusability of this component for different button configurations that don't match these specific names.Consider whether this validation should be more flexible to allow for different use cases:
- selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + selectedName: PropTypes.string,Alternatively, verify that this component will only be used with these specific button types throughout the codebase.
🏁 Script executed:
#!/bin/bash # Search the codebase for uses of ButtonGroupBeta to verify button name values rg -A 3 -B 3 "ButtonGroupBeta" --type js --type jsxLength of output: 79
Action Required: Verify PropTypes for ButtonGroupBeta's
selectedNameThe current PropTypes validation restricts
selectedNameto a fixed set of button type names. Although this can help enforce consistency, it might limit the component's reusability if future use cases require button names outside of this predefined set.
- Next Steps:
- Review Component Usages: Please run the above script (or verify manually) to confirm whether the component is used exclusively with these specific button types throughout the codebase.
- Adjust if Needed: If you expect use cases with additional or dynamic button names, consider relaxing the PropTypes validation from:
Otherwise, if the strict enforcement aligns with the intended usage, no changes might be necessary.- selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + selectedName: PropTypes.string,
🏁 Script executed:
#!/bin/bash # Correctly search for instances of ButtonGroupBeta in .jsx files using a glob pattern rg -A 3 -B 3 "ButtonGroupBeta" -g "*.jsx"Length of output: 4794
Once you run this script, please verify whether the literal button names passed to
selectedNamematch the predefined list (['regular', 'wide', 'full', ...]). This information is crucial to determine if we can safely relax the PropTypes validation without breaking expected behavior.If the usage in stories or elsewhere confirms that only the predefined values are being used, you may choose to keep the current strict validation. On the other hand, if you anticipate dynamic or additional values, consider updating the PropTypes to a broader validation (e.g.,
PropTypes.string).Please review the output to determine the appropriate action.
🏁 Script executed:
#!/bin/bash # Search for literal selectedName values in stories for ButtonGroupBeta usage rg "selectedName:" packages/koenig-lexical/src/components/ui/ButtonGroupBeta.stories.jsxLength of output: 197
Action: Reevaluate the
selectedNamePropTypes Restriction in ButtonGroupBetaThe current PropTypes restrict
selectedNameto an extensive list of values, but our findings show that in the ButtonGroupBeta story only a subset—specifically['regular', 'wide', 'full']—is actually controlled. Additionally, while the SettingsPanel passes a dynamic prop forselectedName, it isn’t clear if values beyond this subset are expected or supported.Please verify the following before deciding on the approach:
- Usage in the Codebase: Confirm that whenever a dynamic or different button name is supplied (e.g., via SettingsPanel), it adheres to one of the allowed names or if new types might be introduced.
- Component Intention: If flexibility for arbitrary button names is desired, consider updating the PropTypes from:
Otherwise, if the current restricted set is intentional, ensure that all usages (including story controls) are updated or documented to reflect this design.- selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + selectedName: PropTypes.string,packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (3)
6-15: LGTM: Clear import structure for new beta components.The imports for beta versions of components are well-organized alongside their original counterparts.
238-248: LGTM: Clean implementation of ButtonGroupSettingBeta.Good implementation that follows the same pattern as the original ButtonGroupSetting but integrates the beta component variant.
262-272: LGTM: Consistent implementation of ColorOptionSettingBeta.The implementation follows the same pattern as the original, using the beta version of ColorOptionButtons while preserving the same structure and props.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (3)
7-27: Consider improving key uniqueness for mapped componentsThe ButtonGroupBeta component looks well-structured, but there's a potential issue with the key generation approach:
key={`${name}-${label}`}This might not guarantee uniqueness if multiple buttons happen to have the same name and label combination. Consider using a more unique identifier for the key.
- key={`${name}-${label}`} + key={`button-${name}-${label}-${index}`}Also, add the index parameter to the map function:
- {buttons.map(({label, name, Icon, dataTestId, ariaLabel}) => ( + {buttons.map(({label, name, Icon, dataTestId, ariaLabel}, index) => (
29-49: Enhance button accessibility with aria-pressedThe component handles button states correctly but could benefit from improved accessibility.
For toggle buttons, it's recommended to add the
aria-pressedattribute to indicate whether the button is currently pressed or not, which helps screen readers communicate the button state to users.<button aria-label={ariaLabel || label} + aria-pressed={isActive} 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'}`} data-testid={dataTestId} type="button" onClick={handleClick} onMouseDown={handleMousedown} >
37-42: Consider extracting complex class name logicThe class names string contains complex conditional logic that could be extracted for better readability and maintainability.
+ const buttonClasses = [ + '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' + ].filter(Boolean).join(' '); <button aria-label={ariaLabel || label} - 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={buttonClasses} data-testid={dataTestId} type="button" onClick={handleClick} onMouseDown={handleMousedown} >packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.stories.jsx (1)
36-43: Consider renaming “desc” prop for clarity
Using “desc” everywhere may reduce readability. A more descriptive name like “description” or “label” can make the intent clear.Here’s an example diff for the “Image” story (the same approach applies to other stories using “desc”):
-Image.args = { - icon: 'image', - desc: 'Click to select an image', - size: 'medium', - borderStyle: 'squared' -}; +Image.args = { + icon: 'image', + description: 'Click to select an image', + size: 'medium', + borderStyle: 'squared' +};packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (1)
118-140: Restrict borderStyle prop to a known set
To prevent mismatched values, consider makingborderStylea union type and removing the “dashed” references if it’s no longer supported:- borderStyle: PropTypes.string, + borderStyle: PropTypes.oneOf(['squared', 'rounded']),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/MediaPlaceholderBeta.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/ButtonGroupBeta.stories.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 (17)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (3)
1-6: Clean, well-organized imports!The imports are organized with external dependencies first, followed by internal components. This is a good practice for maintainability.
1-62: Well-structured component with good separation of concernsOverall, this is a well-designed component that follows React best practices:
- Functional components with clean props destructuring
- Good separation of parent and child components
- Proper use of hooks for side effects
- Support for accessibility via aria attributes
- Proper dark mode support
- Tooltips for better UX
The component fits well with the PR objectives of enhancing UI components in the Koenig editor.
51-52:❓ Verification inconclusive
Verify selectedName validation against dynamic button names
The PropTypes validation restricts
selectedNameto a predefined set of values, but this might be too restrictive if the button names are dynamically generated or come from different sources.Consider whether the strict validation of
selectedNamematches how the component will be used in practice. If button names can vary beyond this predefined list, you might want to use a more flexible approach:- selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']), + selectedName: PropTypes.string,
🏁 Script executed:
#!/bin/bash # Check if there are instances where ButtonGroupBeta is used with button names not in the predefined list # Find all imported/referenced component instances rg -A 3 -B 3 "ButtonGroupBeta" --glob "*.jsx" --glob "*.js" | grep -E "selectedName|name" # Also check for any custom button names that wouldn't match the predefined list rg -A 1 -B 1 "buttons\s*=\s*\[" --glob "*.jsx" --glob "*.js" | grep -E "name\s*:\s*['\"]"Length of output: 742
Attention Required: Validate 'selectedName' PropType for Dynamic Button Names
The current PropTypes validation limits
selectedNameto a specific set of values. However, our initial searches only confirmed that ButtonGroupBeta is used with the hardcoded list in its definition and a usage in SettingsPanel. There was no evidence from the automated search that button names are dynamically generated. That said, if you expect the button names to come from varied or external sources, you might want to consider a more flexible approach—such as switching toPropTypes.string.
- File:
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(Lines 51-52)
- Existing:
selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']),- Proposed (if dynamic button names are possible):
selectedName: PropTypes.string,Please verify the intended usage of
selectedNameacross the codebase to ensure that the set of predefined values covers all valid cases or if a flexible string-based validation is more appropriate.packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.stories.jsx (9)
1-4: No issues found in import statements
Everything looks good here.
5-27: Story configuration looks good
The title, component reference, and argTypes are properly configured for Storybook.
30-34: No issues found in the Template definition
The Template component correctly spreads the args to render the placeholder.
44-51: Same observation regarding “desc”
Consider aligning with the naming suggestion from the “Image” story lines.
52-59: Same observation regarding “desc”
Consider aligning with the naming suggestion from the “Image” story lines.
60-67: Same observation regarding “desc”
Consider aligning with the naming suggestion from the “Image” story lines.
68-75: Same observation regarding “desc”
Consider aligning with the naming suggestion from the “Image” story lines.
76-83: Same observation regarding “desc”
Consider aligning with the naming suggestion from the “Image” story lines.
84-91: Thanks for avoiding the built-in “Error” name
Renaming this story to "ErrorState" successfully prevents shadowing the global Error object.packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (5)
1-11: Imports look good
All necessary modules and assets are properly imported.
12-35: Function signature and props destructuring
This block cleanly initializes the component's props and default values.
36-45: No issues in the file input ref handling
Setting the fileInputRef and calling setFileInputRef conditionally is clear.
47-50: No issues with the onRemove handler
Stopping the event propagation is logical here to maintain card select state.
52-78: Placeholder rendering logic is sound
Checking for empty state and rendering the placeholder plus the upload form is straightforward.
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx
Outdated
Show resolved
Hide resolved
No ref - Added a divider to separate the button settings from the other design settings
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (1)
12-115: Add test coverage for new interaction scenarios.
The new component includes features like file removal, image editing viaopenImageEditor, and anadditionalActionsslot. Consider adding unit or integration tests for these scenarios to ensure stable behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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 (1)
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (1)
80-115: Unify inconsistentborderStyleusage.
This code referencesborderStyle === 'dashed'for one style andborderStyle === 'rounded'for another, while the default is'squared'. This repeats a past review concern, so be sure to consolidate the styles and remove references to'dashed'if it’s not required, or handle each valid style consistently.A sample approach might look like this:
-<div className={clsx('group/image relative flex items-center justify-center', borderStyle === 'dashed' && 'rounded', className)} ...> +<div className={clsx( + 'group/image relative flex items-center justify-center', + borderStyle === 'rounded' && 'rounded-lg', + className +)} ...> -<img alt={alt} - className={clsx( - 'mx-auto size-full rounded-lg', - ... - )} -/> +<img alt={alt} + className={clsx( + 'mx-auto size-full', + borderStyle === 'rounded' && 'rounded-lg', + ... + )} +/> -<div className={clsx('absolute inset-0 rounded-lg bg-gradient-to-t ...', borderStyle === 'dashed' && 'rounded-lg')} ...> +<div className={clsx( + 'absolute inset-0 bg-gradient-to-t ...', + borderStyle === 'rounded' && 'rounded-lg' +)} ...>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (1)
29-49: 🛠️ Refactor suggestionAdd PropTypes for ButtonGroupIconButton component.
You've defined PropTypes for ButtonGroupBeta but not for ButtonGroupIconButton. For consistency and better type safety, add PropTypes for ButtonGroupIconButton as well.
Add these PropTypes at the end of the file:
ButtonGroupIconButton.propTypes = { dataTestId: PropTypes.string, onClick: PropTypes.func.isRequired, label: PropTypes.string, ariaLabel: PropTypes.string, name: PropTypes.string.isRequired, selectedName: PropTypes.string, Icon: PropTypes.elementType, hasTooltip: PropTypes.bool };
🧹 Nitpick comments (6)
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (2)
16-16: Consider a default alt text for accessibility.
If thealtprop isn’t provided, consider providing a meaningful default to improve screen reader accessibility.
54-77: Add unit tests for rendering the placeholder in empty state.
When!isLoading && !src, the component renders a placeholder. Consider adding or expanding test coverage to ensure the placeholder logic and the accompanyingMediaPlaceholderBetausage behave as expected.packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (4)
13-13: Consider using onlynameas the key for ButtonGroupIconButton.Using a composite key of
key={${name}-${label}}might cause issues iflabelis undefined or if two buttons have the same name but one doesn't have a label. Sincenameshould be unique within a button group, consider using it alone as the key.- key={`${name}-${label}`} + key={name}
38-38: Consider breaking down the complex class string for better readability.The long Tailwind class string is difficult to read and maintain. Consider extracting it into separate variables or using a classnames library for better organization.
- 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={` + 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'} + `}
45-45: Add safety check for tooltip label.When
IconandhasTooltipare true butlabelis an empty string, a tooltip will still be displayed without content. Add a check to ensure the label has content.- {(Icon && label && hasTooltip) && <Tooltip label={label} />} + {(Icon && label && hasTooltip && label.trim()) && <Tooltip label={label} />}
32-32: Consider implementing error handling for the onClick callback.The
usePreviousFocushook is used to process click events, but there's no error handling mechanism if theonClickfunction throws an error. Consider wrapping the onClick handler with a try/catch block.- const {handleMousedown, handleClick} = usePreviousFocus(onClick, name); + const {handleMousedown, handleClickRaw} = usePreviousFocus(onClick, name); + + const handleClick = (event) => { + try { + handleClickRaw(event); + } catch (error) { + console.error('Error in button click handler:', error); + } + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx(1 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 (4)
packages/koenig-lexical/src/components/ui/MediaUploaderBeta.jsx (3)
21-21: Unify the handling ofborderStyle.
Currently, the default is'squared', but in the rendered code, the image element always applies'rounded-lg'. This contradicts the'squared'border style. Consider removing or making'rounded-lg'conditional so that'squared'truly renders a non-rounded border.
92-101: Guard against missingopenImageEditor.
WhenisPinturaEnabled === truebutopenImageEditoris undefined, clicking the “Edit” button can cause runtime errors. Consider ensuringopenImageEditoris required or conditionally disabling the edit action in this scenario.
118-141: All prop types are accurately declared.
The addedMediaUploaderBeta.propTypesare complete and reflective of the destructured props, reducing the risk of runtime errors. Great job!packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (1)
7-27: Good implementation of the ButtonGroupBeta component.The component effectively maps through button configurations and renders them as ButtonGroupIconButton components. The use of semantic HTML (ul/li) for the button group is a good practice.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (3)
29-51: 🛠️ Refactor suggestionNeed PropTypes for ButtonGroupIconButton component.
You've defined PropTypes for ButtonGroupBeta but not for ButtonGroupIconButton. This is inconsistent and reduces type safety.
Add the following PropTypes definition after the component:
ButtonGroupIconButton.propTypes = { dataTestId: PropTypes.string, onClick: PropTypes.func.isRequired, label: PropTypes.string, ariaLabel: PropTypes.string, name: PropTypes.string.isRequired, selectedName: PropTypes.string, Icon: PropTypes.elementType, hasTooltip: PropTypes.bool };
10-10: 🛠️ Refactor suggestionIncorrect ARIA role used for button group.
The UL element is using
role="menubar"which is not the most appropriate role for this interaction pattern. Since these are toggle buttons in a group, consider usingrole="tablist"for the container and adjust child elements accordingly.- <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white" role="menubar"> + <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white" role="tablist">
41-42: 🛠️ Refactor suggestionIncorrect ARIA role for button elements.
The role
menuitemradioisn't appropriate for this UI pattern. Since this is a toggle button in a group and the parent has arole="menubar", you should update the roles to follow proper accessibility patterns.- role="menuitemradio" + role="tab"And update the
aria-checkedattribute toaria-selectedto match the tab role:- aria-checked={isActive} + aria-selected={isActive}
🧹 Nitpick comments (4)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx (4)
1-6: Consider adding an explanatory comment for the component.This is a new component with an important role in the UI. Adding a brief JSDoc comment would help other developers understand its purpose and usage.
+/** + * ButtonGroupBeta - A component for rendering a group of toggleable buttons + * with optional icons and tooltips. Used for selecting from a predefined set of options. + */ import PropTypes from 'prop-types'; import React from 'react';
13-13: Improve key uniqueness for ButtonGroupIconButton.Using a combination of name and label for the key could potentially lead to issues if multiple buttons have the same combination. Consider using a more unique identifier or index if appropriate.
- key={`${name}-${label}`} + key={name}
39-39: Consider extracting complex className into a helper function.The conditional class assignments are getting complex and harder to read. Consider extracting this logic into a helper function or using a classnames library.
+ // Helper function to generate classNames based on props + const getButtonClassNames = (isActive, hasIcon) => { + const baseClasses = "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"; + const activeClasses = isActive ? 'border border-grey-300 bg-white shadow-xs dark:bg-grey-900' : ''; + const iconClasses = hasIcon ? '' : 'text-[1.3rem] font-bold'; + return `${baseClasses} ${activeClasses} ${iconClasses}`; + }; - 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={getButtonClassNames(isActive, Boolean(Icon))}
54-54: Consider using a constant for the selectedName enum values.The allowed values for selectedName are hardcoded in the PropTypes. Consider extracting these to a named constant for better maintainability and reuse.
+const BUTTON_GROUP_OPTIONS = [ + 'regular', 'wide', 'full', 'split', 'center', 'left', + 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive' +]; ButtonGroupBeta.propTypes = { - selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list', 'minimal', 'immersive']).isRequired, + selectedName: PropTypes.oneOf(BUTTON_GROUP_OPTIONS).isRequired,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
| {Icon ? <Icon className="size-4 stroke-2" /> : label} | ||
| {(Icon && label && hasTooltip) && <Tooltip label={label} />} | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential accessibility issue with Icon-only buttons.
When a button only has an icon (no visible text), ensure it always has an accessible name via aria-label. The current implementation falls back to the label if ariaLabel isn't provided, but there's a case where both could be missing.
{Icon ? <Icon className="size-4 stroke-2" /> : label}
{(Icon && label && hasTooltip) && <Tooltip label={label} />}
+ {(Icon && !label && !ariaLabel) && console.warn('ButtonGroupIconButton with Icon is missing both label and ariaLabel props')}
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.