Updated CTA card cardwrapper behavior #1414
Conversation
Ref https://linear.app/ghost/issue/PLG-309/add-static-html-for-new-cta-card - When the CTA card has a background, the cardwrapper should not be wide. When the CTA card does not have a background, the wrapper should be wide and have some extra bottom padding to make the bottom border visible.
Ref https://linear.app/ghost/issue/PLG-309/add-static-html-for-new-cta-card - Added color swatches to the CTA card settings panel - Replaced the hasBackground property with a no-background swatch - Added a tooltip to the color picker in callout, signup, header and CTA cards - Swapped white and grey swatches in the callout card
|
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/CardWrapper.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. WalkthroughThe pull request introduces a series of UI enhancements across multiple components in the Koenig Lexical package. Key changes include the addition of a Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant ColorPicker
participant Tooltip
User->>ColorPicker: Hover over color button
ColorPicker->>Tooltip: Trigger tooltip display
Tooltip-->>User: Show descriptive label
User->>ColorPicker: Select color
ColorPicker->>UI: Update component color
Poem
✨ 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: 1
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (1)
182-182: Consider extracting complex className logic.The className conditions are becoming complex. Consider extracting them into a separate function or utility for better maintainability.
+const getWrapperClassName = (color, isEditing, isSelected, hasSponsorLabel) => { + const baseClasses = 'w-full rounded-lg border'; + const colorClass = CALLOUT_COLORS[color]; + const paddingClass = (isEditing || isSelected) && (color === 'none' && !hasSponsorLabel) + ? 'py-3' + : color === 'none' + ? 'pb-3' + : ''; + return `${baseClasses} ${colorClass} ${paddingClass}`; +}; + -<div className={`w-full rounded-lg border ${CALLOUT_COLORS[color]} ${(isEditing || isSelected) && (color === 'none' && !hasSponsorLabel) ? 'py-3' : color === 'none' ? 'pb-3' : ''}`}> +<div className={getWrapperClassName(color, isEditing, isSelected, hasSponsorLabel)}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.jsx(4 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(3 hunks)
🔇 Additional comments (15)
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (2)
3-3: LGTM! Added Tooltip component import.The addition of the Tooltip component enhances accessibility by providing visual feedback for color options.
41-41: LGTM! Improved button styling and accessibility.The changes:
- Use consistent size classes for better maintainability
- Add tooltips to improve accessibility by providing visual labels
Also applies to: 48-50
packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx (3)
65-70: LGTM! Implemented conditional wrapper style based on background.The implementation aligns with the PR objectives by making the wrapper wide only when there's no background color.
75-86: Consider enabling dark mode preview.The commented-out dark mode preview section could be valuable for testing the component's appearance in dark mode.
Would you like me to help verify the dark mode styling implementation?
98-98: LGTM! Updated story configurations.The story configurations have been updated to:
- Use color prop instead of hasBackground
- Include more realistic example content
- Update button text to match the context
Also applies to: 108-108, 112-112, 114-114
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (3)
11-11: LGTM! Restored grey color support.Added grey color definitions for both background and text colors.
Also applies to: 26-26
41-81: LGTM! Enhanced color styling with improved contrast.Updated color definitions with:
- Increased background opacity from 10% to 15%
- Added consistent border styling with improved contrast
- Dark mode support in border colors
160-160: LGTM! Improved UI text and PropTypes.
- Simplified label from 'Background color' to 'Background'
- Reordered color PropTypes for better readability
Also applies to: 175-175
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (2)
6-6: LGTM! Enhanced ColorSwatch accessibility and styling.The changes:
- Add tooltips for better accessibility
- Update button styling with group class for hover states
Also applies to: 123-123, 132-132
159-166: LGTM! Added tooltip to color picker button.Added tooltip with 'Pick color' label to improve accessibility.
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
11-21: Well-structured color definitions!The
CALLOUT_COLORSconstant provides a comprehensive set of color options with appropriate opacity levels and dark mode support.
23-59: Excellent color picker configuration!The
calloutColorPickerconstant provides a clear and accessible UI representation of color options, with thoughtful visual indicators like the strike-through for the 'none' option.packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
239-239: Refined padding for better vertical alignment.The padding adjustment from
pt-1topt-[.6rem]provides more precise control over the spacing in stacked layouts.packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (1)
Line range hint
410-423: Enhanced accessibility with tooltip.The addition of the Tooltip component and group styling improves the user experience by providing clear context for the image toggle button.
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (1)
Line range hint
408-421: Consistent tooltip implementation.The Tooltip addition maintains UI consistency with the HeaderCard component, providing a unified user experience across different card types.
| const visibilitySettings = ( | ||
| <> | ||
| <p className="text-sm font-bold tracking-normal text-grey-900 dark:text-grey-300">Web</p> | ||
| <ToggleSetting | ||
| label="Anonymous visitors" | ||
| /> | ||
| <ToggleSetting | ||
| label="Free members" | ||
| /> | ||
| <ToggleSetting | ||
| label="Paid members" | ||
| /> | ||
| <hr className="not-kg-prose my-2 block border-t-grey-300 dark:border-t-grey-900" /> | ||
| <p className="text-sm font-bold tracking-normal text-grey-900 dark:text-grey-300">Email</p> | ||
| <ToggleSetting | ||
| label="Free members" | ||
| /> | ||
| <ToggleSetting | ||
| label="Paid members" | ||
| /> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Implement onChange handlers for visibility toggles.
The visibility settings section contains toggle controls without onChange handlers, making them non-functional. These should either be implemented or removed if not needed.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (1)
160-181:⚠️ Potential issueImplement onChange handlers for visibility toggles.
The visibility settings section contains toggle controls without onChange handlers, making them non-functional. These should either be implemented or removed if not needed.
🧹 Nitpick comments (2)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
185-185: Simplify complex conditional className logic.The className string contains multiple nested conditions that make it difficult to understand and maintain. Consider extracting this logic into a helper function or using a className utility library like
clsxorclassnames.-<div className={`w-full rounded-lg border ${CALLOUT_COLORS[color]} ${(isEditing || isSelected) && (color === 'none' && !hasSponsorLabel) ? 'py-3' : color === 'none' ? 'pb-3' : ''}`}> +<div className={getCardWrapperClasses({color, isEditing, isSelected, hasSponsorLabel})}>Add this helper function:
function getCardWrapperClasses({color, isEditing, isSelected, hasSponsorLabel}) { const baseClasses = 'w-full rounded-lg border'; const colorClasses = CALLOUT_COLORS[color]; const paddingClasses = color === 'none' ? ((isEditing || isSelected) && !hasSponsorLabel) ? 'py-3' : 'pb-3' : ''; return [baseClasses, colorClasses, paddingClasses].filter(Boolean).join(' '); }
266-267: Mark required callback props.The
updateLayoutandhandleColorChangecallbacks should be marked as required since they are essential for the component's functionality.- updateLayout: PropTypes.func, - handleColorChange: PropTypes.func + updateLayout: PropTypes.func.isRequired, + handleColorChange: PropTypes.func.isRequired
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(2 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(4 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx
- packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx
- packages/koenig-lexical/src/components/ui/SettingsPanel.jsx
- packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
| hasBackground: PropTypes.bool, | ||
| hasImage: PropTypes.bool, | ||
| hasSponsorLabel: PropTypes.bool, | ||
| color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']), |
There was a problem hiding this comment.
Update color prop type to match available colors.
The color prop type validation doesn't match all colors defined in CALLOUT_COLORS. This could lead to runtime errors if pink or purple colors are used.
- color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']),
+ color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red', 'pink', 'purple']),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']), | |
| color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red', 'pink', 'purple']), |
| <div className={`block ${layout === 'immersive' ? 'w-full' : 'w-16 shrink-0'}`}> | ||
| <img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src="https://images.unsplash.com/photo-1511556532299-8f662fc26c06?q=80&w=4431&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D" /> | ||
| <img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for images and interactive elements.
- The image uses a generic "Placeholder" alt text which doesn't provide meaningful information for screen readers.
- Consider adding ARIA labels for interactive elements in the settings panel.
- <img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} />
+ <img
+ alt={buttonText || "CTA card image"}
+ className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`}
+ src={imageSrc}
+ />📝 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.
| <img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} /> | |
| <img | |
| alt={buttonText || "CTA card image"} | |
| className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} | |
| src={imageSrc} | |
| /> |
| export const CALLOUT_COLORS = { | ||
| none: 'bg-transparent border-transparent', | ||
| grey: 'bg-grey/10 border-transparent', | ||
| white: 'bg-transparent border-grey-300 dark:border-grey-800', | ||
| blue: 'bg-blue/10 border-transparent', | ||
| green: 'bg-green/10 border-transparent', | ||
| yellow: 'bg-yellow/10 border-transparent', | ||
| red: 'bg-red/10 border-transparent', | ||
| pink: 'bg-pink/10 border-transparent', | ||
| purple: 'bg-purple/10 border-transparent' | ||
| }; | ||
|
|
||
| export const calloutColorPicker = [ | ||
| { | ||
| label: 'None', | ||
| name: 'none', | ||
| color: 'bg-transparent border-black/15 relative after:absolute after:left-1/2 after:top-1/2 after:h-[1px] after:w-[18px] after:-translate-x-1/2 after:-translate-y-1/2 after:-rotate-45 after:bg-red-500' | ||
| }, | ||
| { | ||
| label: 'White', | ||
| name: 'white', | ||
| color: 'bg-transparent border-black/15 dark:border-white/10' | ||
| }, | ||
| { | ||
| label: 'Grey', | ||
| name: 'grey', | ||
| color: 'bg-grey/15 border-black/[.08] dark:border-white/10' | ||
| }, | ||
| { | ||
| label: 'Blue', | ||
| name: 'blue', | ||
| color: 'bg-blue/15 border-black/[.08] dark:border-white/10' | ||
| }, | ||
| { | ||
| label: 'Green', | ||
| name: 'green', | ||
| color: 'bg-green/15 border-black/[.08] dark:border-white/10' | ||
| }, | ||
| { | ||
| label: 'Yellow', | ||
| name: 'yellow', | ||
| color: 'bg-yellow/15 border-black/[.08] dark:border-white/10' | ||
| }, | ||
| { | ||
| label: 'Red', | ||
| name: 'red', | ||
| color: 'bg-red/15 border-black/[.08] dark:border-white/10' | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Resolve color options inconsistency between constants.
CALLOUT_COLORS includes 'pink' and 'purple' options that are not available in calloutColorPicker. This mismatch could lead to undefined behavior if these colors are somehow selected.
Either:
- Add the missing colors to
calloutColorPicker:
export const calloutColorPicker = [
// ... existing colors ...
+ {
+ label: 'Pink',
+ name: 'pink',
+ color: 'bg-pink/15 border-black/[.08] dark:border-white/10'
+ },
+ {
+ label: 'Purple',
+ name: 'purple',
+ color: 'bg-purple/15 border-black/[.08] dark:border-white/10'
+ }
];- Or remove unused colors from
CALLOUT_COLORS:
export const CALLOUT_COLORS = {
// ... existing colors ...
- pink: 'bg-pink/10 border-transparent',
- purple: 'bg-purple/10 border-transparent'
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const CALLOUT_COLORS = { | |
| none: 'bg-transparent border-transparent', | |
| grey: 'bg-grey/10 border-transparent', | |
| white: 'bg-transparent border-grey-300 dark:border-grey-800', | |
| blue: 'bg-blue/10 border-transparent', | |
| green: 'bg-green/10 border-transparent', | |
| yellow: 'bg-yellow/10 border-transparent', | |
| red: 'bg-red/10 border-transparent', | |
| pink: 'bg-pink/10 border-transparent', | |
| purple: 'bg-purple/10 border-transparent' | |
| }; | |
| export const calloutColorPicker = [ | |
| { | |
| label: 'None', | |
| name: 'none', | |
| color: 'bg-transparent border-black/15 relative after:absolute after:left-1/2 after:top-1/2 after:h-[1px] after:w-[18px] after:-translate-x-1/2 after:-translate-y-1/2 after:-rotate-45 after:bg-red-500' | |
| }, | |
| { | |
| label: 'White', | |
| name: 'white', | |
| color: 'bg-transparent border-black/15 dark:border-white/10' | |
| }, | |
| { | |
| label: 'Grey', | |
| name: 'grey', | |
| color: 'bg-grey/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Blue', | |
| name: 'blue', | |
| color: 'bg-blue/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Green', | |
| name: 'green', | |
| color: 'bg-green/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Yellow', | |
| name: 'yellow', | |
| color: 'bg-yellow/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Red', | |
| name: 'red', | |
| color: 'bg-red/15 border-black/[.08] dark:border-white/10' | |
| } | |
| ]; | |
| export const CALLOUT_COLORS = { | |
| none: 'bg-transparent border-transparent', | |
| grey: 'bg-grey/10 border-transparent', | |
| white: 'bg-transparent border-grey-300 dark:border-grey-800', | |
| blue: 'bg-blue/10 border-transparent', | |
| green: 'bg-green/10 border-transparent', | |
| yellow: 'bg-yellow/10 border-transparent', | |
| red: 'bg-red/10 border-transparent', | |
| pink: 'bg-pink/10 border-transparent', | |
| purple: 'bg-purple/10 border-transparent' | |
| }; | |
| export const calloutColorPicker = [ | |
| { | |
| label: 'None', | |
| name: 'none', | |
| color: 'bg-transparent border-black/15 relative after:absolute after:left-1/2 after:top-1/2 after:h-[1px] after:w-[18px] after:-translate-x-1/2 after:-translate-y-1/2 after:-rotate-45 after:bg-red-500' | |
| }, | |
| { | |
| label: 'White', | |
| name: 'white', | |
| color: 'bg-transparent border-black/15 dark:border-white/10' | |
| }, | |
| { | |
| label: 'Grey', | |
| name: 'grey', | |
| color: 'bg-grey/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Blue', | |
| name: 'blue', | |
| color: 'bg-blue/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Green', | |
| name: 'green', | |
| color: 'bg-green/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Yellow', | |
| name: 'yellow', | |
| color: 'bg-yellow/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Red', | |
| name: 'red', | |
| color: 'bg-red/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Pink', | |
| name: 'pink', | |
| color: 'bg-pink/15 border-black/[.08] dark:border-white/10' | |
| }, | |
| { | |
| label: 'Purple', | |
| name: 'purple', | |
| color: 'bg-purple/15 border-black/[.08] dark:border-white/10' | |
| } | |
| ]; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
78-79: Consider consistent naming pattern for handler props.The prop
handleColorChangebreaks the naming convention used by other update handlers (e.g.,updateLayout,updateButtonText). Consider renaming it toupdateColorfor consistency.- handleColorChange + updateColor
185-192: Simplify conditional classes.The conditional classes for padding could be simplified using a more declarative approach.
- 'py-3': (isEditing || isSelected) && color === 'none' && !hasSponsorLabel, - 'pb-3': color === 'none' && !((isEditing || isSelected) && !hasSponsorLabel) + 'py-3': color === 'none' && ((isEditing || isSelected) && !hasSponsorLabel), + 'pb-3': color === 'none' && !(isEditing || isSelected || hasSponsorLabel)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/koenig-lexical/src/assets/icons/kg-layout-immersive.svgis excluded by!**/*.svgpackages/koenig-lexical/src/assets/icons/kg-layout-minimal.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (5)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (5)
12-60: Resolve color options inconsistency between constants.The
CTA_COLORSincludes 'pink' and 'purple' options that are not available inctaColorPicker. This mismatch could lead to undefined behavior.
160-181: Implement onChange handlers for visibility toggles.The visibility settings section contains toggle controls without onChange handlers, making them non-functional.
214-221: Improve image accessibility.The image uses a generic "Placeholder" alt text which doesn't provide meaningful information for screen readers.
281-281: Update color prop type to match available colors.The color prop type validation doesn't match all colors defined in
CTA_COLORS.
185-208: Implementation successfully addresses PR objectives.The conditional classes effectively modify the cardwrapper behavior based on the presence of a background (color), ensuring:
- Non-wide wrapper when background color is present
- Wide wrapper with visible bottom border when no background
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (3)
11-12: Consider adding color contrast validation.The imported color utilities (
getAccentColorandtextColorForBackgroundColor) are used for determining text colors, but there's no validation to ensure WCAG color contrast compliance.Consider adding color contrast validation to ensure accessibility standards are met, especially for the button text colors against their backgrounds.
84-84: Consider lifting button color picker state.The button color picker state is managed locally but could be lifted up to make the component more reusable and allow external control of the picker's expanded state.
- const [buttonColorPickerExpanded, setButtonColorPickerExpanded] = useState(false);Add these to the component props:
+ buttonColorPickerExpanded: PropTypes.bool, + onButtonColorPickerExpandedChange: PropTypes.func,
213-220: Simplify conditional styling logic.The conditional styling for padding could be simplified using a more declarative approach.
- 'py-3': (isEditing || isSelected) && color === 'none' && !hasSponsorLabel, - 'pb-3': color === 'none' && !((isEditing || isSelected) && !hasSponsorLabel) + 'py-3': color === 'none' && ((isEditing || isSelected) && !hasSponsorLabel), + 'pb-3': color === 'none' && !(isEditing || isSelected || hasSponsorLabel)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (4)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (4)
188-209: Implement onChange handlers for visibility toggles.The visibility settings section contains toggle controls without onChange handlers, making them non-functional. These should either be implemented or removed if not needed.
315-315: Update color prop type to match available colors.The color prop type validation doesn't match all colors defined in
CTA_COLORS. This could lead to runtime errors if pink or purple colors are used.
213-236: Implementation aligns with PR objectives.The conditional styling successfully implements the requirement to modify the cardwrapper behavior based on the presence of a background. When there's no background (
color === 'none'), the wrapper includes additional padding and borders as specified.
242-249: 🛠️ Refactor suggestionImprove image accessibility.
The image uses a generic "Placeholder" alt text which doesn't provide meaningful information for screen readers.
- alt="Placeholder" + alt={buttonText || "CTA card image"}Likely invalid or redundant comment.
Ref https://linear.app/ghost/issue/PLG-309/add-static-html-for-new-cta-card - Added indicatorPosition prop to CardWrapper to allow for custom positioning of the indicator icon
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx (3)
47-56: Consider moving default position outside component.While the position calculation logic is clean, the default position object is recreated on each render. Consider moving it outside the component for better performance.
+const DEFAULT_INDICATOR_POSITION = { + top: '.6rem' +}; export const CardWrapper = React.forwardRef(({ // ... props }) => { - // Default positioning values - const defaultPosition = { - top: '.6rem' - }; // Merge provided position with defaults const position = { - ...defaultPosition, + ...DEFAULT_INDICATOR_POSITION, ...(indicatorPosition || {}) };
62-68: Consider adding accessibility attributes to the indicator icon.The icon positioning looks good, but consider adding an aria-label or title attribute to improve accessibility for screen readers.
<IndicatorIcon className="absolute left-[-6rem] size-5 text-grey" + aria-label={`${cardType} indicator`} style={{ left: position.left, top: position.top }} />
91-95: Consider adding defaultProps for indicatorPosition.While the PropTypes are well-defined, consider adding defaultProps for
indicatorPositionto match the default values used in the component.CardWrapper.defaultProps = { - cardWidth: 'regular' + cardWidth: 'regular', + indicatorPosition: { + top: '.6rem' + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/koenig-lexical/src/assets/icons/kg-indicator-visibility.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/ui/cards/CtaCard.stories.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (1)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx (1)
17-17: LGTM! Clear and descriptive prop name.The new
indicatorPositionprop is well-placed and follows the component's existing prop pattern.
Ref https://linear.app/ghost/issue/PLG-309/add-static-html-for-new-cta-card - Move DEFAULT_INDICATOR_POSITION outside component to prevent recreation on render - Add aria-label to IndicatorIcon for better screen reader support - Add indicatorPosition to defaultProps for consistency
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx (2)
51-54: Consider memoizing the position object.While the position merging logic is clean, the object is recreated on every render. Consider memoizing it with useMemo if the parent component frequently re-renders.
+const position = React.useMemo(() => ({ + ...DEFAULT_INDICATOR_POSITION, + ...(indicatorPosition || {}) +}), [indicatorPosition]); -const position = { - ...DEFAULT_INDICATOR_POSITION, - ...(indicatorPosition || {}) -};
91-94: Consider adding more specific PropType validation.While the shape validation is good, consider adding more specific validation for the position values to ensure they are valid CSS units.
indicatorPosition: PropTypes.shape({ - left: PropTypes.string, - top: PropTypes.string + left: function(props, propName, componentName) { + if (props[propName] && !/^-?\d*\.?\d+(px|rem|em|%)$/.test(props[propName])) { + return new Error( + `Invalid prop ${propName} supplied to ${componentName}. Validation failed. Expected a valid CSS unit.` + ); + } + }, + top: function(props, propName, componentName) { + if (props[propName] && !/^-?\d*\.?\d+(px|rem|em|%)$/.test(props[propName])) { + return new Error( + `Invalid prop ${propName} supplied to ${componentName}. Validation failed. Expected a valid CSS unit.` + ); + } + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (3)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx (3)
13-15: LGTM! Well-structured default constant.The
DEFAULT_INDICATOR_POSITIONconstant is well-defined and follows good practices by centralizing the default positioning value.
60-67: LGTM! Clean implementation of dynamic positioning.The IndicatorIcon styling implementation is clean and properly applies the dynamic position values.
21-21: Verify integration with CTA card background conditions.The new
indicatorPositionprop provides the flexibility needed for the CTA card requirements. Please ensure that the CTA card component correctly sets this prop based on the presence of a background.Also applies to: 98-99
Ref https://linear.app/ghost/issue/PLG-309/add-static-html-for-new-cta-card
Summary by CodeRabbit
New Features
Bug Fixes
Style