Added toggle to Call to Action card to show/hide dividers #1509
Conversation
Ref https://linear.app/ghost/issue/PROD-1628/add-setting-to-showhide-dividers - This supports the use case where the card is used as inline with text, and the dividers are not needed.
WalkthroughThis change introduces a new boolean property, 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/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.jsOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
310-311: Consider simplifying complex conditional styling logic.The conditional logic for padding and borders is quite complex with nested conditions that may be difficult to maintain as the component evolves.
Consider extracting these conditions into helper functions or computed values to improve readability:
+ // At the top of the component or as a separate utility + const shouldShowTopBorder = (color, showDividers, hasSponsorLabel, hasImmersiveImage) => { + return (color === 'none' && showDividers) || (hasSponsorLabel && !hasImmersiveImage); + }; + + const shouldShowBottomBorder = (color, showDividers) => { + return (color === 'none' && showDividers); + }; + + // Inside the component + const hasImmersiveImage = imageSrc && layout === 'immersive'; // Then in your JSX <div className={clsx( 'flex gap-6', - hasSponsorLabel && color !== 'none' && (imageSrc && layout === 'immersive') ? '' : (color === 'none' && !showDividers && !hasSponsorLabel) ? '' : 'pt-6', - imageSrc && !showButton ? 'pb-8' : (color === 'none' && !showDividers) ? '' : 'pb-7', + !hasImmersiveImage || !hasSponsorLabel || color === 'none' ? ((color === 'none' && !showDividers && !hasSponsorLabel) ? '' : 'pt-6') : '', + imageSrc && !showButton ? 'pb-8' : (color === 'none' && !showDividers) ? '' : 'pb-7', layout === 'immersive' ? 'flex-col' : 'flex-row', - (color === 'none' && showDividers) || (hasSponsorLabel && !(imageSrc && layout === 'immersive')) ? 'border-t border-grey-900/15 dark:border-grey-100/20' : '', - (color === 'none' && showDividers) ? 'border-b border-grey-900/15 dark:border-grey-100/20' : color !== 'none' ? 'mx-6' : '' + shouldShowTopBorder(color, showDividers, hasSponsorLabel, hasImmersiveImage) ? 'border-t border-grey-900/15 dark:border-grey-100/20' : '', + shouldShowBottomBorder(color, showDividers) ? 'border-b border-grey-900/15 dark:border-grey-100/20' : color !== 'none' ? 'mx-6' : '' )}>This approach makes the code more maintainable and the conditions more explicit.
Also applies to: 348-349, 351-352
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-parser.js(2 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(3 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(7 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (13)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
14-14: Good addition of the new property.The new
showDividersproperty is properly defined with a default value oftrue, which aligns with the PR objective to add a toggle for showing/hiding dividers in the Call to Action card.packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
114-114: Correct prop passing to component.The new
showDividersproperty is correctly passed down to theCallToActionNodeComponent, which enables the divider toggle functionality in the UI.packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-parser.js (2)
23-23: Proper detection of divider class.Good implementation of detecting the
kg-cta-has-dividersclass to determine the value of theshowDividersproperty during parsing.
51-51: Correctly added property to payload.The
showDividersproperty is properly added to the payload object, which ensures it will be correctly set when creating a new node instance.packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (3)
28-28: Proper conditional class application.The
kg-cta-has-dividersCSS class is correctly applied conditionally based on theshowDividersproperty, which will control the visibility of dividers in the rendered HTML.
182-182: Consistent implementation in email template.The conditional class is also applied correctly in the email template, ensuring consistent behavior between web and email renderings.
209-209: Property correctly added to dataset.The
showDividersproperty is properly included in the dataset object, making it available for both rendering methods.packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (3)
26-26: Implementation looks good!The
showDividersproperty is correctly added to the component props, allowing the toggle functionality to be passed through to the component.
62-67: Well-implemented toggle function.The
toggleShowDividersfunction follows the same pattern as other toggle functions in this file, maintaining consistency in the codebase.
187-187: Correct prop passing implementation.The
showDividersprop and its update function are correctly passed to theCallToActionCardcomponent, completing the integration of the new feature.Also applies to: 199-199
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (3)
102-102: Props correctly defined with sensible defaults.The
showDividersprop defaults totrue, which maintains backward compatibility with existing content, and theupdateShowDividersfunction has a sensible empty function default.Also applies to: 116-116
254-261: Good conditional rendering of the dividers toggle.The dividers toggle is correctly shown only when the background color is 'none', which is the only scenario where dividers are relevant. The implementation follows the same pattern as other toggle settings in this component.
449-449: Props properly defined in PropTypes.Good job adding the new
showDividersandupdateShowDividersproperties to the PropTypes definition, ensuring proper type checking and documentation.Also applies to: 457-457
Ref https://linear.app/ghost/issue/PROD-1628/add-setting-to-showhide-dividers - Changed classname from kg-cta-has-dividers to kg-cta-no-dividers to ensure backwards compatibility
1 participant
Ref https://linear.app/ghost/issue/PROD-1628/add-setting-to-showhide-dividers