Align new settings panels with other cards #1486
Conversation
WalkthroughThe pull request introduces significant modifications across various UI components and their associated tests. A new prop, Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/src/utils/callToActionColors.jsOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.jsOops! 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
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx (1)
49-51: Consider expanding PropTypes definitionWhile the PropTypes for selectedName are defined correctly, the component also accepts other props (buttons, onClick) that should have PropTypes defined to ensure proper usage.
ButtonGroup.propTypes = { - selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list']) + selectedName: PropTypes.oneOf(['regular', 'wide', 'full', 'split', 'center', 'left', 'small', 'medium', 'large', 'grid', 'list']), + buttons: PropTypes.arrayOf(PropTypes.shape({ + label: PropTypes.string.isRequired, + name: PropTypes.string.isRequired, + Icon: PropTypes.elementType, + dataTestId: PropTypes.string + })), + onClick: PropTypes.func };Similarly, consider adding PropTypes for the IconButton component.
packages/koenig-lexical/src/components/ui/MediaPlaceholder.stories.jsx (1)
6-6: Suggest consistent naming across all stories.
Renaming the story to include "(beta)" is fine if this component is indeed in beta, but ensure consistency with other beta or stable components across the codebase.packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
218-218: Disable tooltips intentionally?
SettinghasTooltip={false}helps keep the UI simpler. Double-check if the user needs a tooltip for layout choices.packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
8-13: Good approach for tracking and closing popover.
Storing the currently selected button and usinguseClickOutsideto close ensures intuitive interactions. Consider graceful fallback if no button matches theselectedName(e.g., undefined case).packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (1)
146-146: Update the type propType definition for clarityThe type propType is defined as oneOf(['image', 'button']), but in the component logic you use conditional checks like
type === 'button'vstype !== 'button'. Consider updating either the propType definition or the component logic to ensure perfect alignment.- type: PropTypes.oneOf(['image', 'button']), + type: PropTypes.string,Alternatively, if you want to maintain the strict validation:
- {type === 'button' - ? <ButtonContents desc={desc} hasErrors={errors.length > 0} /> - : <StandardContents desc={desc} hasErrors={errors.length > 0} icon={icon} size={size} /> - } + {type === 'button' + ? <ButtonContents desc={desc} hasErrors={errors.length > 0} /> + : <StandardContents desc={desc} hasErrors={errors.length > 0} icon={icon} size={size} /> + }packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.stories.jsx (1)
84-85: Avoid shadowing the global Error propertyThe story name "Error" shadows the global JavaScript Error property/constructor, which could lead to confusion.
-export const Error = Template.bind({}); -Error.args = { +export const ErrorExample = Template.bind({}); +ErrorExample.args = {🧰 Tools
🪛 Biome (1.9.4)
[error] 84-84: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
243-246: Commented code should be removed.There's a commented line of code that appears to be no longer needed. Clean code practices recommend removing commented code rather than leaving it in the codebase.
await page.click('[data-testid="color-options-button"]'); await page.click('[data-testid="background-image-color-button"]'); -// await page.click('[data-testid="background-image-color-button"]');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
packages/koenig-lexical/src/components/ui/ButtonGroup.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.old.stories.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx(0 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtons.old.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsx(0 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.old.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.old.stories.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.stories.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ColorPickerBeta.jsx(0 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx(4 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.stories.jsx(3 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.stories.jsx(2 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.jsx(0 hunks)packages/koenig-lexical/src/components/ui/MediaUploader.jsx(6 hunks)packages/koenig-lexical/src/components/ui/MediaUploader.old.jsx(6 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(4 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx(0 hunks)packages/koenig-lexical/src/components/ui/cards/FileCard.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(2 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(2 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(2 hunks)packages/koenig-lexical/test/e2e/cards/callout-card.test.js(4 hunks)packages/koenig-lexical/test/e2e/cards/email-cta-card.test.js(1 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(9 hunks)packages/koenig-lexical/test/e2e/cards/signup-card.test.js(7 hunks)packages/koenig-lexical/test/utils/background-color-helper.js(0 hunks)packages/koenig-lexical/test/utils/color-select-helper.js(1 hunks)
💤 Files with no reviewable changes (6)
- packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx
- packages/koenig-lexical/src/components/ui/ColorPickerBeta.jsx
- packages/koenig-lexical/src/components/ui/MediaPlaceholderBeta.jsx
- packages/koenig-lexical/src/components/ui/ButtonGroupBeta.jsx
- packages/koenig-lexical/src/components/ui/ColorOptionButtonsBeta.jsx
- packages/koenig-lexical/test/utils/background-color-helper.js
🧰 Additional context used
🧬 Code Definitions (13)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (2)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.old.jsx (3)
ColorOptionButtons(6-31)ColorButton(33-54)isActive(34-34)packages/koenig-lexical/src/components/ui/ColorPicker.jsx (1)
isOpen(141-141)
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (1)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (2)
MediaUploadSetting(275-305)ColorPickerSetting(246-273)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.old.jsx (3)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (4)
ColorOptionButtons(7-70)ColorButton(72-93)isActive(73-73)usePreviousFocus(75-75)packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (2)
isActive(30-30)usePreviousFocus(32-32)packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx (2)
isActive(28-28)usePreviousFocus(30-30)
packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (1)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (2)
MediaUploadSetting(275-305)ColorPickerSetting(246-273)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.jsx (1)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (6)
PLACEHOLDER_ICONS(11-18)PLACEHOLDER_ICONS(11-18)CardText(20-30)CardText(20-30)MediaPlaceholder(67-140)Icon(44-44)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (2)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.jsx (6)
CardText(19-23)CardText(19-23)Icon(39-39)PLACEHOLDER_ICONS(10-17)PLACEHOLDER_ICONS(10-17)MediaPlaceholder(25-71)packages/koenig-lexical/src/components/ui/Button.jsx (1)
props(21-27)
packages/koenig-lexical/test/e2e/cards/callout-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (1)
selectNamedColor(1-6)
packages/koenig-lexical/src/components/ui/ButtonGroup.stories.jsx (2)
packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (1)
ButtonGroup(7-27)packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx (1)
ButtonGroup(7-25)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (4)
MediaUploadSetting(275-305)ButtonGroupSetting(222-232)ColorOptionSetting(234-244)ColorPickerSetting(246-273)
packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (3)
selectNamedColor(1-6)selectCustomColor(8-15)selectTitledColor(17-22)
packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (3)
packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx (3)
ButtonGroup(7-25)isActive(28-28)usePreviousFocus(30-30)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (2)
isActive(73-73)usePreviousFocus(75-75)packages/koenig-lexical/src/components/ui/Tooltip.jsx (1)
Tooltip(3-12)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (2)
color(2-2)color(18-18)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
packages/koenig-lexical/src/components/ui/ColorPicker.old.jsx (6)
ColorPicker(9-100)stopPropagation(13-28)ColorIndicator(137-170)backgroundColor(103-103)backgroundColor(138-138)selectedSwatch(139-139)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
isOpen(8-8)packages/koenig-lexical/src/components/ui/Tooltip.jsx (1)
Tooltip(3-12)
🪛 Biome (1.9.4)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.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)
🔇 Additional comments (102)
packages/koenig-lexical/src/components/ui/ColorPicker.stories.jsx (1)
5-5: Update of story title looks good.The story title has been updated to include "(New)" which helps differentiate it from the older version in Storybook navigation.
packages/koenig-lexical/src/components/ui/cards/FileCard.jsx (1)
5-7: Improved import paths with file extensions.Adding the
.jsxextensions to the import paths makes the code more explicit and helps with module resolution.packages/koenig-lexical/src/components/ui/ColorPicker.old.stories.jsx (2)
2-6: Component reference properly updated.The story has been updated to use the stable
ColorPickercomponent instead of the beta version, which aligns with the removal of beta designations across the codebase.
21-21: Component usage updated consistently.The template rendering is now using the stable
ColorPickercomponent, maintaining consistency with the imports and component references.packages/koenig-lexical/test/e2e/cards/email-cta-card.test.js (2)
331-337: Improved accessibility with proper ARIA attributes.The addition of
role="menubar"to the<ul>element andaria-checked,aria-label, androle="menuitemradio"to the button elements significantly improves accessibility for users relying on assistive technologies.
343-347: Consistent accessibility improvements for the Center button.The Center button now correctly includes accessibility attributes matching those of the Left button, with appropriate
aria-checked="false"value to indicate its unselected state.packages/koenig-lexical/src/components/ui/ButtonGroup.old.stories.jsx (3)
5-5: Updated import to use stable component versionsThe import statement has been updated to use the stable versions of
ButtonGroupandIconButtoncomponents from './ButtonGroup' instead of their beta counterparts, supporting the migration from beta components to production-ready components.
8-10: Updated component references and story configurationThe story title and component references have been updated to reflect the stable versions:
- Removed "(beta)" from the title
- Updated the main component reference to
ButtonGroup- Updated subcomponents to include
IconButtonThese changes align with the migration from beta components to stable ones.
24-24: Updated template renderingThe template now renders the stable
ButtonGroupcomponent instead of the beta version, completing the migration to stable components across the story file.packages/koenig-lexical/src/components/ui/ButtonGroup.old.jsx (2)
7-25: Well-structured ButtonGroup component implementationThe ButtonGroup component is cleanly implemented with a clear responsibility of rendering a collection of IconButton components. The component uses appropriate semantic HTML elements (div and ul) with proper class styling.
27-47: IconButton component follows best practicesThe IconButton component is well-implemented with:
- Clear state management for active selection
- Proper use of the usePreviousFocus hook for handling interactions
- Appropriate ARIA attributes for accessibility
- Conditional rendering of tooltips when both Icon and label are provided
- Clean styling with proper class naming
This implementation aligns well with modern React patterns and accessibility standards.
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (2)
430-430: Improved background handling with explicit 'image' valueThe value prop for ColorPickerSetting has been updated to use 'image' when a background image is displayed, making the code more expressive and self-documenting compared to the previous empty string approach.
452-478: Improved component hierarchy with nested media upload controlMoving the MediaUploadSetting inside ColorPickerSetting creates a more logical component hierarchy, as the image upload is directly related to the background choice in the color picker. This structuring:
- Improves semantic relationship between components
- Better groups related UI elements
- Allows the ColorPickerSetting to manage its children's visibility
This change aligns with the card UI standardization goal mentioned in the PR objectives.
packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (3)
431-431: Improved background handling with explicit 'image' valueThe value prop for ColorPickerSetting has been updated to use 'image' when a background image is displayed, making the code more expressive and self-documenting compared to the previous empty string approach.
437-437: Consistent function parameter formattingThe onTogglePicker callback formatting has been standardized by removing extra spacing, improving code style consistency.
453-479: Improved component hierarchy with nested media upload controlMoving the MediaUploadSetting inside ColorPickerSetting creates a more logical component hierarchy, as the image upload is directly related to the background choice in the color picker. This structuring:
- Improves semantic relationship between components
- Better groups related UI elements
- Allows the ColorPickerSetting to manage its children's visibility
This change aligns with the card UI standardization goal mentioned in the PR objectives.
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (3)
387-387: Update data attribute name for consistent testing conventionThe attribute has been updated from
data-test-idtodata-testid, which aligns with the standardized testing attribute naming convention being implemented across the codebase. This change improves consistency with other components and testing utilities like the newly addedcolor-select-helper.js.
420-420: Update data attribute name for consistent testing conventionThe attribute has been updated from
data-test-idtodata-testid, which improves testing selector consistency. This change is part of a broader effort to standardize test attribute naming throughout the codebase.
426-426: Update data attribute name for consistent testing conventionThe attribute has been updated from
data-test-idtodata-testid, maintaining consistency with the other changes in this file and across the codebase.packages/koenig-lexical/src/components/ui/ColorOptionButtons.old.jsx (1)
1-55: Refactored component aligns with updated design patternsThis newly created
.old.jsxfile follows the naming convention for legacy components while maintaining the updated attribute naming (data-testid). The implementation properly uses theusePreviousFocushook and includes accessibility features likearia-label.The file organization aligns with the PR objective of standardizing UI components. Note that the suffix
.old.jsxsuggests this is being preserved as part of the refactoring strategy while newer versions are being implemented elsewhere.packages/koenig-lexical/test/utils/color-select-helper.js (1)
1-22: Good abstraction of color selection testing logicThis new utility file provides excellent abstraction for color selection in tests, which will improve maintainability and readability. The functions properly use the standardized
data-testidattribute format.The implementation of three specialized functions (
selectNamedColor,selectCustomColor, andselectTitledColor) provides flexibility for different color selection scenarios while keeping the test code DRY. This abstraction aligns well with the PR's goal of standardizing UI components and testing patterns.packages/koenig-lexical/test/e2e/cards/callout-card.test.js (3)
4-5: Uncommented import for color selection helperThe import for
selectNamedColorhas been properly uncommented, allowing the use of this abstracted helper function throughout the test file.
146-156: Improved test maintainability with helper functionThe direct selector interaction has been replaced with the abstracted
selectNamedColorhelper function, improving code maintainability and readability. The commented code shows the previous approach for context.This change is part of the broader effort to standardize testing patterns and attribute naming throughout the codebase, making tests more consistent and easier to maintain.
328-329: Consistent approach to color selection in testsSimilar to the previous change, this replaces direct selector interaction with the
selectNamedColorhelper function, maintaining consistency with other color selection operations in the test suite.packages/koenig-lexical/src/components/ui/MediaPlaceholder.stories.jsx (1)
84-85: Clearer naming for the error story.
RenamingErrortoErrorStateimproves clarity, distinguishing this scenario from other potential error-related stories.packages/koenig-lexical/src/components/ui/ButtonGroup.stories.jsx (1)
5-5: No major issues with the beta naming and imports.
The changes, including importingButtonGroupIconButtonand marking the story as beta, align with the updated refactoring strategy.Also applies to: 8-8, 10-10
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (5)
10-10: Stable settings panel imports.
Replacing old beta imports with stable ones fromSettingsPanel.jsxis consistent with the removal of beta components.
170-170: Media upload setting usage looks good.
TheMediaUploadSettingintegration with thetype="button"prop and relevant handlers appears correct. Make sure invalid media types are handled upstream or within this component.
226-226: Background color option setting.
Swapping to the stableColorOptionSettingis consistent with your refactor. No further concerns.
234-234: Link color option setting.
Likewise, replacing the beta version withColorOptionSettingis aligned with your updated approach.
242-261: Enhanced button color picker usage.
The expanded color picker (with eyedropper support) provides a better user experience. Great work ensuring continuity between the background and text color.packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (4)
2-4: New imports for state management and click-outside detection.
IntroducinguseStateanduseClickOutsideis a solid approach for managing popover visibility and closing it on outside clicks.
17-36: Popover container structure.
Using a ref and a visually appealing conic gradient background add clarity for the selected color. This approach is well-structured.
38-67: Dynamically rendered color options.
Mapping over color buttons and closing the popover upon selection increases usability. Great job ensuring consistent test IDs.
77-81: Class and test ID refinements.
Addingmb-0and unifying data-testid attributes reflect attention to detail. This consistency eases testing and styling.packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (10)
9-9: Good addition of clsx dependencyThe clsx utility is a great choice for conditionally joining classNames together, which improves readability in the component's conditional styling.
20-30: Nice enhancement to CardText for flexibilityThe CardText component has been nicely updated to support conditional styling based on the type prop. This aligns with the PR objective of consistent card styling across settings panels.
32-37: Well-structured ButtonContents componentThis new component nicely encapsulates button-specific rendering logic, improving code organization.
39-65: Good extraction of StandardContents componentThe StandardContents component effectively extracts and encapsulates the standard content rendering logic, making the main component more readable. The use of clsx for conditional class application is clean and maintainable.
72-72: Good addition of the type propThe new type prop enhances component flexibility, aligning with the PR objective of consistent card styling.
82-89: Well-structured container class conditionalsThe containerClasses implementation with clsx effectively handles multiple conditional styles based on type, size, and borderStyle props.
91-95: Clear buttonClasses implementationThe buttonClasses logic effectively handles different styling needs based on the type and size props.
97-100: Clean errorClasses implementationThe error styling is well-separated and consistently applied.
102-110: Good extraction of error messages mappingExtracting the error message mapping logic improves readability in the main component render method.
119-136: Well-structured conditional renderingThe conditional rendering based on isDraggedOver and type prop is clean and easy to follow, enhancing component flexibility.
packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.stories.jsx (3)
3-3: Good update to import the renamed componentThe import has been correctly updated to use the stable MediaPlaceholder component instead of the beta version.
6-7: Appropriate story title updateThe story title has been updated to reflect that this is now a stable component rather than a beta version.
32-32: Correct component usage in TemplateThe Template now correctly uses the MediaPlaceholder component.
packages/koenig-lexical/src/components/ui/MediaPlaceholder.old.jsx (1)
1-79: Well-preserved legacy componentThis file appears to maintain the original implementation of MediaPlaceholder for backward compatibility. The code structure and implementation are consistent with the updated version, which will make future maintenance easier.
packages/koenig-lexical/src/components/ui/MediaUploader.old.jsx (7)
7-7: Updated import to use the stable componentThe import has been correctly updated to use the stable MediaPlaceholder component.
12-34: Component parameters updated for clarityThe component signature has been updated to remove the type prop and make onRemoveMedia required. This simplifies the interface and makes it more intuitive.
56-57: Updated to use the stable MediaPlaceholder componentThe JSX now correctly uses the stable MediaPlaceholder component without passing a type prop.
79-79: Simplified container class structureThe class names for the container have been simplified and now use clsx for conditional application, which improves readability.
82-82: Cleaned up image class structureThe class names for the image element have been simplified and use clsx for conditional application.
106-106: Simplified loading overlay class structureThe class names for the loading overlay have been cleaned up to remove unnecessary conditions.
116-136: Improved PropTypes definitionThe PropTypes have been updated to better reflect the component's props, with proper shape definitions for nested objects.
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (8)
5-5: Good refactoring to use helper functions for color selectionThe import of helper functions
selectCustomColorandselectTitledColorfrom the color-select-helper module is a good improvement that will help standardize color selection across tests.
206-210: Improved test selectors using data-testid attributesThe update from aria-label selectors to data-testid attributes (
data-testid="color-selector-button") is a good practice that makes tests more robust against UI text changes.
217-218: Simplified color selection with helper functionUsing the
selectCustomColorhelper function makes the test more maintainable and consistent with other tests. The additional click on settings panel ensures proper UI state before assertions.
229-240: Streamlined background color testingThe changes follow the same pattern of using the helper function for color selection and clicking on the settings panel to ensure UI updates are complete before assertions.
245-265: Added comprehensive test for color variantsGood addition of a test case that verifies different background color options (grey, black, and brand color) and validates both background color and text color updates for each change.
275-277: Updated image upload flow for new UIThe commented-out code and updated click sequence reflect the changes in the UI flow for background image selection, maintaining test coverage while adapting to the new interface.
362-368: Improved color testing with better visual validationReplacing direct button clicks with the color selector pattern and using the helper functions creates a more consistent testing approach. The immediate assertions on container properties improve test reliability.
377-380: Standardized image toggle interactionThe updated interaction pattern for toggling between color and image backgrounds ensures the tests reflect the actual user flow in the updated UI.
packages/koenig-lexical/src/components/ui/ColorPicker.old.jsx (1)
1-171: Preserving legacy component for referenceThis entire file represents the previous implementation of the ColorPicker components, now archived with the
.old.jsxsuffix. Keeping this reference during the transition period is good practice for maintaining institutional knowledge and providing a fallback if needed.packages/koenig-lexical/src/components/ui/MediaUploader.jsx (6)
20-20: Added type prop for UI flexibilityAdding the
typeprop enables the component to render differently based on context (e.g., 'image' or 'button'), which enhances its reusability.
32-32: Added default function for onRemoveMediaGood defensive programming by providing a default empty function for
onRemoveMedia, preventing potential null reference errors when the callback is not provided.
68-68: Passing type to MediaPlaceholderCorrectly passing the new
typeprop to the MediaPlaceholder component ensures consistent rendering behavior throughout the component hierarchy.
81-86: Improved responsive layout with conditional stylingThe addition of conditional minimum width based on loading state and better class organization improves the component's visual consistency across different states.
89-89: Enhanced image responsivenessAdding
min-w-[5.2rem]class to the image element ensures it maintains a consistent minimum width, preventing layout shifts during loading or when handling small images.
123-145: Updated PropTypes for better type safetyThe reorganized and expanded PropTypes definition more accurately reflects the component's API, improving developer experience and documentation. The addition of the
typeprop with specific allowed values enhances type safety.packages/koenig-lexical/src/components/ui/ColorPicker.jsx (8)
2-3: Added imports for enhanced functionalityGood additions:
ImgBgIconfor supporting image backgroundsuseStatefor managing component stateuseClickOutsidehook for improving UX of the color picker popoverThese additions support the expanded functionality of the component.
Also applies to: 9-9
11-11: Enhanced ColorPicker with children supportAdding the
childrenprop and updating the component structure allows for more flexible composition, enabling additional content to be rendered inside the color picker. The removal of margin-top class makes the component more adaptable to different contexts.Also applies to: 81-100
140-151: Improved ColorIndicator with popover functionalityGood enhancements:
- Added state variables for managing UI state (
isOpen,showChildren)- Added ref for popover DOM access
- Implemented click-outside handling for better UX
- Added event handling to prevent propagation
These changes create a more interactive and user-friendly color selection experience.
158-161: Added support for image background typeThe addition of the 'image' type handling is an important feature that supports background image selection, aligning with the PR's goal of standardizing settings panels across different card types.
170-174: Added handler to maintain popover state during color selectionThe
handleColorPickerChangefunction properly updates the color value without closing the popover, improving the user experience by allowing continuous color adjustments.
176-205: Redesigned color selector buttonThe button has been completely redesigned with:
- Better accessibility through
data-testidattribute- Improved visual appearance with conditional styling
- Support for displaying the selected color, including the new image type
- Proper state management for toggling the popover
These improvements create a more polished and consistent UI.
207-277: Implemented comprehensive popover UIThe new popover implementation:
- Uses proper refs and event handling
- Supports both swatches and custom color picker
- Has responsive width based on content
- Includes proper organization of color options
- Provides clear toggle controls
This significantly improves the user experience for color selection while maintaining consistency with other UI components.
199-202: Visual indication for image backgroundsGood addition of the
ImgBgIconfor image backgrounds, providing clear visual feedback to users about the selected background type. This helps distinguish between solid colors and image backgrounds in a consistent way.packages/koenig-lexical/test/e2e/cards/header-card.test.js (10)
5-5: Good addition of color selection helper functions.Adding these utility functions improves code reusability and maintainability by abstracting the color selection logic that was previously duplicated across test files.
189-191: Enhanced accessibility testing with aria attributes.Switching from testing CSS classes to checking
aria-checkedattributes is a better practice that focuses on the semantic state of the UI components rather than their visual appearance.
198-201: Consistent accessibility testing approach.Using
aria-checkedattributes for testing the medium button state maintains consistency with the approach used for the small button.
211-211: Consistent aria attribute testing.Continuing the pattern of using
aria-checkedfor the large button maintains a consistent approach to testing button states.
223-232: Improved test readability with helper functions.Replacing direct DOM interactions with the
selectNamedColorhelper function makes the tests more concise and easier to understand, while maintaining the same functionality.
521-526: Effective use of helper function for color selection.Using the
selectCustomColorhelper function simplifies the test code and makes it more maintainable compared to direct DOM manipulation.
541-546: Good test structure for background color.The test correctly sets up the color selection, applies the change, and then clicks outside to close the panel, which accurately reflects real user behavior.
559-579: Excellent new test for color options.This new test significantly improves coverage by verifying that the header card correctly handles different background colors and applies the appropriate text colors for contrast. The use of
selectTitledColormakes the test concise and readable.
587-590: Clear interaction sequence for background image setting.The test clearly demonstrates the sequence of interactions needed to set a background image, which is important for documenting the expected behavior.
627-628: Well-structured test for file upload.Moving the file chooser promise setup earlier in the test makes the code flow more logical and easier to follow.
packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (8)
7-7: Good addition of hasTooltip prop with sensible default.Adding the
hasTooltipprop with a default value oftruemaintains backward compatibility while adding the flexibility to disable tooltips when needed, such as in space-constrained UIs.
10-10: Enhanced accessibility with proper ARIA role.Adding
role="menubar"to the container improves accessibility by correctly identifying the control's purpose to screen readers and other assistive technologies.
11-22: Improved component interface with ariaLabel and hasTooltip props.Adding the
ariaLabelprop to the button configuration and passinghasTooltipto the button component provides more control over accessibility features and tooltip behavior.
29-29: Clear naming with ButtonGroupIconButton.Renaming the component from
IconButtontoButtonGroupIconButtonmakes the relationship between components clearer and reduces the risk of naming conflicts.
37-39: Enhanced accessibility with ARIA attributes.Adding
aria-checkedfor state andaria-labelwith a fallback improves the component's accessibility significantly. The conditional class application for active state is also well implemented.
41-41: Appropriate ARIA role for toggleable buttons.Changing to
role="menuitemradio"better represents the button's function as a selectable option within a group, improving accessibility.
47-47: Smart conditional rendering of tooltip.The condition
(Icon && label && hasTooltip)ensures tooltips only appear when all necessary conditions are met, preventing empty or unnecessary tooltips.
54-63: Comprehensive prop types update.The prop types have been appropriately updated to include the new options and props, which helps with development-time error checking and documentation.
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (5)
5-7: Good cleanup of beta component imports.Replacing beta component imports with their stable counterparts indicates proper progression through the component lifecycle, from beta to stable release.
Also applies to: 11-11
222-222: Consistent prop interface for ButtonGroupSetting.Adding the
hasTooltipprop toButtonGroupSettingmaintains consistency with the updatedButtonGroupcomponent.
228-228: Proper prop forwarding.Forwarding the
hasTooltipprop to theButtonGroupcomponent correctly implements the tooltip control functionality.
246-246: Enhanced flexibility with children prop.Adding the
childrenprop toColorPickerSettingincreases the component's flexibility, allowing for custom content to be rendered within the color picker.
267-269: Proper children forwarding to ColorIndicator.The
childrenprop is correctly passed to theColorIndicatorcomponent, enabling the flexible content rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/koenig-lexical/test/utils/color-select-helper.js (4)
1-8: Add JSDoc comments and error handling for improved maintainability.This utility function works correctly for selecting a named color in tests, but could be improved with JSDoc comments explaining the parameters and purpose. Consider adding error handling for cases where the color elements aren't found.
+/** + * Selects a named color from the color picker + * @param {Page} page - Playwright page object + * @param {string} colorName - Name of the color to select + * @param {string} [testId] - Test ID of the color element to click first (optional) + */ export async function selectNamedColor(page, colorName, testId) { if (testId) { const color = page.locator(`[data-testid="${testId}"]`); await color.click(); } const colorPicker = page.locator(`[data-testid="color-picker-${colorName}"]`); await colorPicker.click(); }
10-17: Add JSDoc comments and color validation for the custom color function.The function works correctly for selecting a custom color, but would benefit from documentation and validation of the color parameter to ensure it's in a valid format.
+/** + * Selects a custom color by entering a color value + * @param {Page} page - Playwright page object + * @param {string} color - Color value to enter (e.g., "#FF5733") + * @param {string} [pickerTestId] - Test ID of the custom color element to click first (optional) + */ export async function selectCustomColor(page, color, pickerTestId) { if (pickerTestId) { const customColor = page.locator(`[data-testid="${pickerTestId}"]`); await customColor.click(); } + // Validate color format (optional) + if (!color.match(/^#[0-9A-Fa-f]{6}$/)) { + console.warn('Color may not be in a valid hex format: ', color); + } const customColorInput = page.locator(`[aria-label="Color value"]`); await customColorInput.fill(color); }
19-26: Consider refactoring duplicated code and clarify parameter names.This function has significant overlap with
selectNamedColorand could be refactored to reduce duplication. Also, the parameter namecolorNameis misleading since it's used for the title attribute.+/** + * Selects a color from the color picker using the title attribute + * @param {Page} page - Playwright page object + * @param {string} colorName - Title attribute of the color to select + * @param {string} [testId] - Test ID of the color element to click first (optional) + */ -export async function selectTitledColor(page, colorName, testId) { +export async function selectTitledColor(page, colorTitle, testId) { if (testId) { const color = page.locator(`[data-testid="${testId}"]`); await color.click(); } - const colorPicker = page.locator(`[title="${colorName}"]`); + const colorPicker = page.locator(`[title="${colorTitle}"]`); await colorPicker.click(); }You could also consider a more DRY approach by refactoring both functions:
/** * Base function for selecting a color * @param {Page} page - Playwright page object * @param {string} selector - Selector for the color picker * @param {string} [testId] - Test ID of the color element to click first (optional) */ async function selectColor(page, selector, testId) { if (testId) { const color = page.locator(`[data-testid="${testId}"]`); await color.click(); } const colorPicker = page.locator(selector); await colorPicker.click(); } /** * Selects a named color from the color picker */ export async function selectNamedColor(page, colorName, testId) { await selectColor(page, `[data-testid="color-picker-${colorName}"]`, testId); } /** * Selects a color from the color picker using the title attribute */ export async function selectTitledColor(page, colorTitle, testId) { await selectColor(page, `[title="${colorTitle}"]`, testId); }
1-26: Add tests for these helper functions to ensure reliability.Since these are testing utility functions that will be used across multiple test files, I recommend creating tests for these helpers themselves to verify they work correctly with different inputs and edge cases.
You could create a test file like
color-select-helper.test.jsthat uses mocks to verify the functions work as expected:// Example test structure (using Jest and mock-playwright) import { selectNamedColor, selectCustomColor, selectTitledColor } from './color-select-helper'; import { test, expect } from '@playwright/test'; test('selectNamedColor clicks the correct elements', async () => { // Setup mock page with spy methods const mockPage = { locator: jest.fn().mockImplementation((selector) => ({ click: jest.fn().mockResolvedValue(undefined) })) }; await selectNamedColor(mockPage, 'red', 'color-button'); // Verify the correct locators were called expect(mockPage.locator).toHaveBeenCalledWith('[data-testid="color-button"]'); expect(mockPage.locator).toHaveBeenCalledWith('[data-testid="color-picker-red"]'); }); // Similar tests for other functions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(7 hunks)packages/koenig-lexical/test/utils/color-select-helper.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (2)
472-476: Commented out code needs cleanupThere are commented out lines that should be removed to keep the code clean. These lines represent the old implementation approach that has been replaced by the new helper functions.
const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -// await colorOptionsButton.click(); -// await selectTitledColor(page, 'Grey', null); -// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'});
479-481: Inconsistent implementation pattern for color selectionThe pattern for color selection is inconsistent here. When a testId is provided to the
selectTitledColorfunction, there's no need to click the button first. This would be more consistent with the rest of the refactored code.-await colorOptionsButton.click(); -await selectTitledColor(page, 'Black', null); +await selectTitledColor(page, 'Black', 'cta-button-color');packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
243-246: Commented out duplicated codeLine 245 is a commented out duplicate of line 244. This should be removed to keep the code clean.
await page.click('[data-testid="color-options-button"]'); await page.click('[data-testid="background-image-color-button"]'); -// await page.click('[data-testid="background-image-color-button"]');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(7 hunks)packages/koenig-lexical/test/e2e/cards/callout-card.test.js(4 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/test/e2e/cards/callout-card.test.js
🧰 Additional context used
🧬 Code Definitions (2)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (5)
selectNamedColor(1-8)color(3-3)color(21-21)selectTitledColor(19-26)selectCustomColor(10-17)
packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (3)
selectNamedColor(1-8)selectCustomColor(10-17)selectTitledColor(19-26)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (15)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (5)
5-5: Good addition of utility functions for color selection!Adding the color selection helper functions improves code maintainability by centralizing the color selection logic, making the tests more readable and less brittle.
367-367: Simplified array structure improves readabilityThe colors array has been simplified from objects with
testIdandexpectedClassproperties to a simple array of color names. This makes the code cleaner and easier to understand.
376-379: Properly refactored color selection logicThe color selection logic has been refactored to use the new utility functions, making the tests more maintainable and consistent.
434-438: Good replacement of direct UI interactions with utility functionsThe code now uses the
selectTitledColorutility function instead of the previouscardBackgroundColorSettingsapproach. This abstraction improves test maintainability.
457-462: Fixed color picker toggling and custom color selectionThe code now correctly uses the new utility functions for color picker toggling and custom color selection.
packages/koenig-lexical/test/e2e/cards/header-card.test.js (10)
5-5: Good addition of utility functions for color selection!Adding the color selection helper functions improves code maintainability by centralizing the color selection logic, making the tests more readable and less brittle.
189-191: Improved accessibility testing with aria-checked attributeUsing the
aria-checkedattribute for validation is a better practice as it focuses on accessibility attributes rather than implementation details like CSS classes.
198-201: Consistent use of aria-checked for button state validationThe refactored code consistently uses
aria-checkedfor validating button states, which improves the accessibility focus of the tests.
223-232: Properly refactored color selection logicThe background color selection logic has been refactored to use the
selectNamedColorutility function, making the tests more maintainable and consistent.
521-526: Properly refactored color selection for button backgroundThe button background color selection has been refactored to use the utility functions.
541-546: Properly refactored background color selectionThe background color selection logic has been refactored to use the
selectCustomColorutility function.
559-579: Great new test for specific color optionsThis new test thoroughly verifies the ability to change the background color to specific named colors (Grey, Black, Brand color) and checks the resulting CSS properties, enhancing test coverage.
588-590: Simplified image upload interactionThe code now uses a more direct approach for image upload interaction, making the test clearer and more maintainable.
606-608: Consistent use of utility functions for color selectionThe code consistently uses the new utility approach for color selection throughout the file.
613-616: Improved color picker toggling logicThe color picker toggling logic has been improved to use the new utility functions.
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | ||
| await colorOptionsButton.click(); | ||
| await selectTitledColor(page, 'Grey', null); | ||
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('background-color: rgb(240, 240, 240);'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing implementation for button color to grey test
The test for changing button color to grey calls the colorOptionsButton.click() method but then appears to use the old implementation approach in line 449. Line 448 isn't needed since the click action is handled within the selectTitledColor function when a testId is provided.
const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]');
-await colorOptionsButton.click();
-await selectTitledColor(page, 'Grey', null);
+await selectTitledColor(page, 'Grey', 'cta-button-color');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | |
| await colorOptionsButton.click(); | |
| await selectTitledColor(page, 'Grey', null); | |
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('background-color: rgb(240, 240, 240);'); | |
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | |
| await selectTitledColor(page, 'Grey', 'cta-button-color'); | |
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('background-color: rgb(240, 240, 240);'); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
11-11: Extended component signature.
The addedchildrenprop inColorPickerenables flexible nested content. If you anticipate null or undefined usage, consider defaulting this prop or checking its type for safety.
147-150: Potential accessibility consideration.
stopPropagationpluspreventDefaultcan inadvertently suppress keyboard or assistive technology interactions. If applicable, consider adding a fallback for keyboard navigation.
176-207: Button styling and gradient duplication.
The swirl gradient logic is repeated here and elsewhere (e.g., lines 259-265). Consider extracting this styling into a helper or shared CSS class for maintainability./* Example approach: move the gradient and masking styles into a reused CSS class .conic-gradient-swatch */packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
222-222: NewhasTooltipprop for ButtonGroup.
PropagatinghasTooltipallows conditional tooltip rendering. This is a beneficial addition, but consider default values if many components do not require tooltips.Also applies to: 228-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(4 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(2 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx
- packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx
🧰 Additional context used
🧬 Code Definitions (1)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (2)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
isOpen(8-8)packages/koenig-lexical/src/components/ui/Tooltip.jsx (1)
Tooltip(3-12)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (10)
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (7)
2-3: Imports look consistent.
All imported modules (ImgBgIcon,Fragment,useCallback, etc.) appear to be used within the file. This organization is clear and correct.
9-9: Confirm hook integration.
UsinguseClickOutsideseems appropriate for closing the popover when clicking outside. Ensure that its side effects are fully tested (i.e., confirm the popover closes in all intended conditions).
81-81: Event handling and children rendering.
Stopping event propagation at the container level (lines 81) can help prevent inadvertent closure of the picker, but be mindful of potential side effects on nested components (e.g., keyboard events). Rendering{children}(line 99) is consistent with the newchildrenprop.Also applies to: 99-99
140-143: Popover state & click-outside handling.
Introducing[isOpen, setIsOpen]and[showChildren, setShowChildren]provides granularity for controlling complex UI states.useClickOutsidegracefully closes the popover, which aligns with user expectations. No concurrency or performance issues spotted here.Also applies to: 145-145
158-160: Handle 'image' as a special color case.
Treating'image'as a color value by forcing a'transparent'background is a neat approach, but ensure that downstream usage checks for'image'to avoid confusion if it’s truly not a color.
170-173: Color selection callback.
handleColorPickerChangekeeps the popover open after changing the color, which matches the comment “Don’t close the popover.” This is clear, though ensure it’s tested with typical user flows.
210-280: Popover layout and toggling logic.
The conditional rendering ofisOpen,isExpanded, andshowChildrenwithin the popover is well-structured overall, although juggling three booleans can be complex. If the logic grows further, consider extracting sub-components. Currently it’s manageable.packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (3)
5-7: Stable component imports.
Removing beta imports in favor of stable ones (ButtonGroup,ColorIndicator,ColorOptionButtons,MediaUploader) reduces duplication and aligns code with production-ready components.Also applies to: 11-11
246-246:ColorPickerSettingenhancements.
AddingchildrenandshowChildrenhelps create more customizable color-picking flows. Validate that parent components pass these props correctly and that transitions between child states are tested.Also applies to: 261-261
276-276: RefinedMediaUploadSetting.
Updating to the stableMediaUploaderand exposingimgClassNameis consistent with removing beta components. If multiple classes are needed, consider a consolidated approach to reduce repetition.Also applies to: 292-292
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (4)
458-458: Consider using color helper functions for consistency.For better consistency across tests, consider using the color helper functions here instead of direct selectors.
-await page.locator('[data-testid="color-picker-accent"]').click(); +await selectNamedColor(page, 'accent', null);And similarly for line 464:
-await page.locator('[data-testid="color-picker-text"]').click(); +await selectNamedColor(page, 'text', null);Also applies to: 464-464
481-485: Simplify color selection using helper function parameter.The current implementation requires two steps (clicking the button, then selecting color), but the
selectTitledColorfunction allows passing the testId to handle both in one step.-const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -await colorOptionsButton.click(); -await selectTitledColor(page, 'Black', null); +await selectTitledColor(page, 'Black', 'cta-button-color');
504-508: Simplify custom color selection using helper function parameter.Similar to the other color selection refactors, this can be simplified using the testId parameter.
-const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -await colorOptionsButton.click(); -await page.locator('[data-testid="color-picker-toggle"]').click(); -await selectCustomColor(page, 'ff0000', null); +await page.locator('[data-testid="color-picker-toggle"]').click(); +await selectCustomColor(page, 'ff0000', 'cta-button-color');
526-527: Simplify color selection using helper function parameter.Same issue as before - simplify using the testId parameter in the helper function.
-await colorOptionsButton.click(); -await selectTitledColor(page, 'Black', null); +await selectTitledColor(page, 'Black', 'cta-button-color');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx
🧰 Additional context used
🧬 Code Definitions (1)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
packages/koenig-lexical/test/utils/color-select-helper.js (5)
selectNamedColor(1-8)color(3-3)color(21-21)selectTitledColor(19-26)selectCustomColor(10-17)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (2)
5-5: Good addition of color select helper imports.The addition of these utility functions helps standardize color selection throughout the tests.
494-497: Missing implementation for button color to grey test.The test for changing button color to grey calls the
colorOptionsButton.click()method but then appears to use the old implementation approach in line 449. Line 448 isn't needed since the click action is handled within theselectTitledColorfunction when a testId is provided.const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -await colorOptionsButton.click(); -await selectTitledColor(page, 'Grey', null); +await selectTitledColor(page, 'Grey', 'cta-button-color');
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js
Outdated
Show resolved
Hide resolved
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | ||
| // await colorOptionsButton.click(); | ||
| // await selectTitledColor(page, 'Grey', null); | ||
| // await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'}); | ||
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('color: rgb(255, 255, 255);'); |
There was a problem hiding this comment.
Uncomment or remove color selection code for button text color test.
The commented-out code suggests the test should set an initial color, but this step is currently skipped. Either uncomment the color selection code to properly test initial state or remove it if not needed.
const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]');
-// await colorOptionsButton.click();
-// await selectTitledColor(page, 'Grey', null);
-// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'});
+// No color selection needed as we're testing the default state📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | |
| // await colorOptionsButton.click(); | |
| // await selectTitledColor(page, 'Grey', null); | |
| // await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'}); | |
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('color: rgb(255, 255, 255);'); | |
| const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); | |
| // No color selection needed as we're testing the default state | |
| expect(await page.getAttribute('[data-testid="cta-button"]', 'style')).toContain('color: rgb(255, 255, 255);'); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
521-524:⚠️ Potential issueUncomment or remove color selection code
The test for "button text color changes with button color" is currently only testing the default state, but the test name implies it should test color changes. Either uncomment the color selection code to properly test color changes or adjust the test name to reflect what's being tested.
Choose one of these approaches:
- If testing color changes is intended:
const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -// await colorOptionsButton.click(); -// await selectTitledColor(page, 'Grey', null); -// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'}); +await colorOptionsButton.click(); +await selectTitledColor(page, 'Grey', null);
- Or if the test should only check default states:
-const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); -// await colorOptionsButton.click(); -// await selectTitledColor(page, 'Grey', null); -// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'FFFFFF'}); +// Check default button text color (should be white on accent color background)
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
226-231: Using stable ButtonGroupSetting component.Successfully migrated from
ButtonGroupSettingBetatoButtonGroupSetting. This component might have new functionality like thehasTooltipprop as seen in the code snippets, though it's not used here. Consider if tooltip functionality would be beneficial for any of these layout options.packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (2)
483-486: Remove commented-out codeThe implementation using
selectTitledColorlooks good, but there's commented-out code that should be removed.const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); await colorOptionsButton.click(); await selectTitledColor(page, 'Black', null); -// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', findByColorTitle: 'Black'});
506-510: Remove commented-out codeThe custom color selection implementation looks good, but there's commented-out code that should be removed.
const colorOptionsButton = page.locator('[data-testid="cta-button-color"] [data-testid="color-selector-button"]'); await colorOptionsButton.click(); await page.locator('[data-testid="color-picker-toggle"]').click(); await selectCustomColor(page, 'ff0000', null); -// await cardBackgroundColorSettings(page, {cardColorPickerTestId: 'cta-button-color', customColor: 'ff0000'});
📜 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(4 hunks)packages/koenig-lexical/src/utils/callToActionColors.js(1 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(7 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (4)
MediaUploadSetting(276-307)ButtonGroupSetting(222-232)ColorOptionSetting(234-244)ColorPickerSetting(246-274)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (3)
packages/koenig-lexical/test/utils/color-select-helper.js (4)
selectNamedColor(1-8)color(3-3)color(21-21)selectTitledColor(19-26)packages/koenig-lexical/src/utils/callToActionColors.js (2)
CALLTOACTION_COLORS(1-11)CALLTOACTION_COLORS(1-11)packages/koenig-lexical/test/utils/e2e.js (2)
focusEditor(58-61)insertCard(404-417)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (12)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (7)
12-13: Good refactoring to use stable component versions and extract constants.The changes here show two improvements:
- Removing "Beta" suffix from component names, indicating a transition to stable versions
- Moving the
CALLTOACTION_COLORSconstant to a dedicated utility file for better code organizationThis aligns with the PR objective of standardizing the settings panels with other card components.
178-194: Using stable MediaUploadSetting component.The component has been updated from
MediaUploadSettingBetatoMediaUploadSetting, which is part of the overall effort to stabilize the UI components. The props and functionality remain the same.
234-239: Using stable ButtonGroupSetting component for alignment options.The component migration is consistent with the other changes in this file, replacing
ButtonGroupSettingBetawithButtonGroupSetting.
243-249: Updated ColorOptionSetting component for background color selection.The component has been upgraded from
ColorOptionSettingBetatoColorOptionSettingwhile maintaining the same functionality.
251-257: Using stable ColorOptionSetting for link color options.The component migration is consistent with other changes, replacing
ColorOptionSettingBetawithColorOptionSetting.
259-278: Updated ColorPickerSetting component for button color configuration.The component has been migrated from
ColorPickerSettingBetatoColorPickerSettingwhile preserving all the original functionality, including expanded state handling and color transformation logic.
290-306:✅ Verification successful
Verify CALLTOACTION_COLORS reference is working correctly.
The component now uses the imported
CALLTOACTION_COLORSconstant from the utility file instead of a local constant. Make sure this reference works correctly across all color options.
🏁 Script executed:
#!/bin/bash # Check if the CALLTOACTION_COLORS constant is being imported and used correctly rg -A 1 "CALLTOACTION_COLORS" packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsxLength of output: 301
CALLTOACTION_COLORS Reference Verified
The constant is now properly imported from the utility file (
../../../utils/callToActionColors.js) and used in the component. The verification shows that it is correctly applied based on the color options.packages/koenig-lexical/src/utils/callToActionColors.js (1)
1-11: Good extraction of color constants to a dedicated utility file.Moving the
CALLTOACTION_COLORSconstant to a separate utility file follows good coding practices by:
- Separating concerns
- Making the constant reusable across components
- Reducing the size and complexity of the CallToActionCard component
The color definitions use Tailwind classes with transparency values that match the design system.
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (4)
2-2: Great improvement on the imports!Nice work importing the
CALLTOACTION_COLORSconstant and the color selection helper functions. This promotes code reuse and consistency across tests.Also applies to: 6-6
415-415: Good refactoring of color testing approachThe simplified color array and use of the
selectNamedColorhelper function makes the tests more maintainable. Using theCALLTOACTION_COLORSconstant for assertions ensures consistency with the implementation.Also applies to: 424-428
496-498: Fixed implementation for grey button color testThe implementation now correctly uses the
selectTitledColorhelper function, addressing a previous review comment.
528-529: Good usage of color selection helperProperly using the
selectTitledColorhelper function to change the button color to black.
ronaldlangeveld
changed the title
ref https://linear.app/ghost/issue/PLG-374/align-new-settings-panel-settings-with-other-cards
ColorPickerandColorOptionButtons, adding image supportMediaPlaceholderandMediaUploaderfor consistent designSettingsPanelwith updated component integration