Organised CTA card settings panel #1449
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 pull request introduces multiple changes across various UI components. In the call-to-action functionality, the default property values for the button have been updated, ensuring that it is shown by default with preset text and color. Several components have been enhanced by adding new state variables (e.g., for managing color option visibility) and by altering function signatures to accept additional props such as children and type. Styling adjustments include updates to class names in button groups, modifications in color and media placeholder components utilizing conditional class handling, and the expansion of color options in card components. There are also refinements in the settings panels governing color pickers and media uploads, alongside the addition of new CSS classes and a Tailwind CSS box shadow utility. 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/test/e2e/cards/callout-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
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 (
|
![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 (5)
packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (2)
32-82: Consider simplifying size-related class definitions.The class definitions are well-organized, but the size-related classes could be simplified using a mapping object.
Consider refactoring the size-related classes:
+const SIZE_CLASSES = { + large: 'size-20', + small: 'size-14', + xsmall: 'size-5', + default: 'size-16' +}; -const iconClasses = clsx( - 'shrink-0 opacity-80 transition-all ease-linear hover:scale-105 group-hover:opacity-100', - size === 'large' && 'size-20 text-grey', - size === 'small' && 'size-14 text-grey', - size === 'xsmall' && 'size-5 text-grey-700', - !['large', 'small', 'xsmall'].includes(size) && 'size-16 text-grey', - (size === 'xsmall' && desc) && 'mr-3' -); +const iconClasses = clsx( + 'shrink-0 opacity-80 transition-all ease-linear hover:scale-105 group-hover:opacity-100', + SIZE_CLASSES[size] || SIZE_CLASSES.default, + size === 'xsmall' ? 'text-grey-700' : 'text-grey', + (size === 'xsmall' && desc) && 'mr-3' +);
84-144: Consider extracting button components for better maintainability.The render logic is well-implemented, but the button components could be extracted to reduce complexity and improve maintainability.
Consider creating separate components for the different button types:
const ButtonTypeButton = ({desc, errors, errorClasses, errorDataTestId, filePicker}) => ( <button className="group flex cursor-pointer select-none items-center justify-center px-3 py-1" name="placeholder-button" type="button" onClick={filePicker} > {!errors.length && ( <p className="!font-sans !text-[1.3rem] !font-medium text-grey-900">{desc}</p> )} <ErrorMessages errors={errors} errorClasses={errorClasses} errorDataTestId={errorDataTestId} /> </button> ); const ImageTypeButton = ({size, errors, Icon, iconClasses, descriptionClasses, desc, errorClasses, errorDataTestId, filePicker}) => ( <button className={buttonClasses} name="placeholder-button" type="button" onClick={filePicker} > {!(size === 'xsmall' && errors.length > 0) && ( <> <Icon className={iconClasses} /> <p className={descriptionClasses}>{desc}</p> </> )} <ErrorMessages errors={errors} errorClasses={errorClasses} errorDataTestId={errorDataTestId} /> </button> ); const ErrorMessages = ({errors, errorClasses, errorDataTestId}) => ( <> {errors.map(error => ( <span key={error.message} className={errorClasses} data-testid={errorDataTestId} > {error.message} </span> ))} </> );packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (2)
17-24: Consider adding ARIA attributes for accessibility.The color indicator uses complex visual styling but lacks accessibility information for screen readers.
Add ARIA attributes to improve accessibility:
{selectedName && ( <div + role="img" + aria-label={`Selected color: ${selectedButton?.label}`} 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' }} /> )}
31-59: Document the complex CSS gradient implementation.The conic gradient and mask implementation is sophisticated but lacks documentation explaining its purpose and how it works.
Add a comment explaining the gradient implementation:
{/* Color options popover */} +{/* The color indicator uses a conic gradient with mask compositing to create a rainbow border effect. + - The conic-gradient creates a circular rainbow + - The mask/WebkitMask with linear-gradients creates the border effect + - The maskComposite/WebkitMaskComposite determines how the masks interact +*/} {isOpen && (packages/koenig-lexical/src/components/ui/ColorPicker.jsx (1)
178-276: Enhance accessibility of color picker buttons.While the implementation is solid, consider adding ARIA attributes for better screen reader support.
<button className={`relative size-6 cursor-pointer rounded-full ${value ? 'p-[2px]' : 'border border-grey-200 dark:border-grey-800'}`} type="button" + aria-expanded={isOpen} + aria-haspopup="true" + aria-label="Open color picker" onClick={() => { setIsOpen(!isOpen); setShowColorPicker(false); setShowChildren(false); }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 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/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/v2/HeaderCard.jsx(3 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)
🔇 Additional comments (24)
packages/koenig-lexical/tailwind.config.cjs (1)
94-94: LGTM! Well-designed box shadow utility.The new
xsbox shadow utility is a great addition that provides a subtle elevation effect. The value is appropriately positioned betweenDEFAULTandsmshadows, and its light opacity makes it perfect for gentle UI elevation, such as in button groups.packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (2)
9-9: LGTM! Good use of clsx for class name management.The addition of
clsxand the refactoring ofCardTextto support button styling is well-implemented. The conditional class names are clear and maintainable.Also applies to: 20-30
147-157:❓ Verification inconclusive
Verify the impact of removing media type options.
The PropTypes for
typehave been restricted to only 'image' and 'button', removing support for 'gallery', 'video', 'audio', 'file', and 'product'. This could be a breaking change.Run the following script to check for existing usage of the removed types:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of removed media types in the codebase echo "Searching for usage of removed media types..." rg -l "type=['\"](?:gallery|video|audio|file|product)['\"]" --type js --type jsx echo "Searching for direct prop assignments..." ast-grep --pattern 'type={$_}' | rg -i 'gallery|video|audio|file|product'Length of output: 382
Verify the impact of removed media type options
The update restricts the
typeprop to only'image'and'button', effectively removing support for'gallery','video','audio','file', and'product'. This change could break existing functionality if any component still relies on the removed options.Our initial automated search encountered an issue with JSX file types. Please run the updated script below to verify that there are no lingering references to the removed media types in both JavaScript and JSX files:
#!/bin/bash # Improved search for usage of removed media types in .js and .jsx files echo "Searching for usage of removed media types in .js and .jsx files..." rg -l --glob "*.js" --glob "*.jsx" "type=['\"](?:gallery|video|audio|file|product)['\"]" echo "Searching for direct prop assignments via ast-grep..." ast-grep --pattern 'type={$_}' | rg -i 'gallery|video|audio|file|product'Please manually verify these results. If no references are found, the breaking change should be safe. Otherwise, adjust the PropTypes or update usages accordingly.
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
11-14: LGTM! Improved default values for better UX.The changes to default values provide a better out-of-the-box experience:
- Button shown by default
- Meaningful default button text
- Default button color set to black
packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (1)
10-10: LGTM! Enhanced button group styling.The styling changes improve visual consistency and state indication:
- Better background colors for button group container
- Clearer active state with border and shadow
- Proper dark mode support
Also applies to: 36-36
packages/koenig-lexical/src/components/ui/MediaUploader.jsx (1)
20-20: LGTM! Well-implemented type prop addition.The new type prop is properly:
- Added to component parameters
- Passed to MediaPlaceholder
- Validated with PropTypes
Also applies to: 68-68, 125-125
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (1)
46-77: LGTM! Consistent color opacity updates.The opacity updates from 15% to 20% for all colors improve visual consistency across the color palette.
packages/koenig-lexical/src/components/ui/cards/VideoCard.jsx (1)
102-112: LGTM! Improved media upload settings.The changes enhance the media upload interface by:
- Adding a descriptive label with
desc='Upload'- Updating to a button-style interface with
type='button'packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
9-9: LGTM! Enhanced ColorPicker component.The component now supports child elements, allowing for more flexible and extensible color picker interfaces.
Also applies to: 97-97
172-175: LGTM! Improved color picker interaction.The
handleColorPickerChangefunction correctly prevents the popover from closing during color selection.
138-176:❓ Verification inconclusive
Verify state management in ColorIndicator.
The new state variables (
isOpen,showColorPicker,showChildren) and their management throughuseEffectlook good, but ensure they don't cause any race conditions or unintended side effects when rapidly toggling states.
🏁 Script executed:
#!/bin/bash # Search for similar state management patterns in other components ast-grep --pattern $'const [$_,$_] = useState($_); const [$_,$_] = useState($_); const [$_,$_] = useState($_); useEffect(() => { if ($_) { $$$ } }, [$_]);'Length of output: 164
Action Required: Confirm Stability Under Rapid Toggle
The state updates in
ColorIndicatorappear straightforward, with theuseEffectsynchronously settingisOpen,showChildren, andshowColorPickerbased on theisExpandedprop. However, our automated script did not provide conclusive evidence regarding potential race conditions. Please verify manually that rapid toggling ofisExpandeddoes not lead to unintended side effects or race conditions between these state updates.
- Location:
packages/koenig-lexical/src/components/ui/ColorPicker.jsx(lines 138-176)- Focus Areas:
- Ensure that the transitions of
isOpen,showColorPicker, andshowChildrenremain consistent under fast, repeated interactions.- Confirm that no asynchronous side effects or delayed state updates occur that could lead to unstable UI behavior.
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (3)
6-6: LGTM! Simplified imports.Removing the ColorPicker import and using only ColorIndicator aligns with the new color picker implementation.
246-273: LGTM! Enhanced ColorPickerSetting component.The component now supports custom toolbar content and children elements, making it more flexible and reusable.
275-306:❓ Verification inconclusive
Verify the stacked layout behavior.
The new stacked layout logic in MediaUploadSetting looks good, but ensure it works correctly with all possible combinations of props.
🏁 Script executed:
#!/bin/bash # Search for usage of stacked prop in MediaUploadSetting rg -A 5 'stacked={true}|stacked\s*=\s*{[^}]*true[^}]*}' --type jsLength of output: 204
Action Needed: Verify Stacked Layout Combinations in MediaUploadSetting
The automated regex query encountered a syntax error; please run a corrected verification check or manually verify that the conditional class logic in the component works as intended for all prop combinations. In particular, ensure that:
- When
stackedis true, the container omits the flex layout and applies the intended height (e.g.,'h-32').- When
stackedis false, the component dynamically computes the class names based on the presence ofsrcand the value oftype.- All combinations of props result in the expected UI behavior.
A corrected shell script for further automated review might be:
If the automated check returns no output or unexpected matches, please verify manually by testing the component with various prop configurations.
🏁 Script executed:
[run_scripts] #!/bin/bash # Verify conditional class names that depend on the "stacked" prop in the MediaUploadSetting component rg -A 5 'clsx\(([^)]*stacked\s*&&\s?["'\'']h-32["'\'']' packages/koenig-lexical/src/components/ui/SettingsPanel.jsxLength of output: 336
Action Required: Verify MediaUploadSetting's Stacked Layout
The stacked layout logic appears correctly implemented in the JSX; however, given that our automated regex checks returned errors, please manually verify that the component correctly applies:
- The
'h-32'class and omits the flex layout whenstackedis true.- The alternative dynamic classes (depending on the presence of
srcand the value oftype) whenstackedis false.Ensure that all combinations of props render the expected UI for this component.
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (5)
24-26: LGTM! New color options added.The addition of pink and purple background colors enhances the customization options for the CTA card.
48-48: LGTM! Color picker enhanced with new options and improved visibility.The changes include:
- Added pink and purple color options
- Increased color opacity from 10% to 20% for better visibility
Also applies to: 53-53, 58-58, 63-63, 68-68, 70-78
163-163: LGTM! Improved media upload UI.The changes enhance the media upload interaction by:
- Using a button-style interface
- Adding a descriptive label
Also applies to: 169-169
253-253: LGTM! Improved CSS class naming.The addition of consistent class name prefixes enhances maintainability and CSS specificity.
Also applies to: 296-296
348-348: LGTM! Updated props for enhanced functionality.The changes improve the component by:
- Adding type safety for new color options
- Setting button visibility to true by default for better UX
Also applies to: 382-382
packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (2)
418-418: LGTM! Improved color picker interaction.The color picker now automatically expands when toggling background image, providing better UX.
454-479: LGTM! Improved media upload organization.The MediaUploadSetting is now conditionally rendered based on layout and background image visibility, improving component organization.
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (2)
416-416: LGTM! Consistent color picker behavior.The color picker expansion behavior now matches HeaderCard's implementation.
452-478: LGTM! Consistent media upload organization.The MediaUploadSetting's conditional rendering matches HeaderCard's implementation.
packages/koenig-lexical/src/styles/components/kg-prose.css (1)
984-990: LGTM! Improved link styling.The new CSS classes enhance link styling in CTA cards by:
- Using currentColor for better theme support
- Maintaining consistent styling in dark mode
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/test/e2e/cards/callout-card.test.js (1)
319-322: LGTM with a minor cleanup suggestion.The implementation maintains consistency with other color picker interactions. Consider removing the extra blank line at 322 for better code formatting.
const colorSetting = page.locator('[data-testid="callout-color-picker"]'); const colorButton = colorSetting.locator('button'); await colorButton.click(); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(0 hunks)packages/koenig-lexical/test/e2e/cards/callout-card.test.js(2 hunks)
💤 Files with no reviewable changes (1)
- packages/koenig-lexical/src/nodes/CallToActionNodeComponent.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/test/e2e/cards/callout-card.test.js (1)
142-145: LGTM! Well-structured color picker interaction.The new implementation better reflects the actual DOM structure and user interaction flow, making the test more maintainable.
Ref https://linear.app/ghost/issue/PLG-355/improve-cta-card-settings-panel