Added Button Colour Handling to CTA card #1418
Conversation
|
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/CallToActionNode.jsOops! Something went wrong! :( ESLint: 8.57.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CtaCard
participant CallToActionNode
participant CallToActionNodeComponent
User->>CtaCard: Select/Change Button Color
CtaCard->>CallToActionNodeComponent: Trigger handleButtonColorChange
CallToActionNodeComponent->>CallToActionNode: Update buttonColor
CallToActionNodeComponent->>CallToActionNode: Update buttonTextColor
CallToActionNode-->>CtaCard: Render Updated Button
Possibly related PRs
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: 0
🧹 Nitpick comments (6)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (2)
12-12: Unify default values forbuttonTextColor.Currently, this property’s default is set to an empty string here, but the constructor fallback is
'none'. Consider unifying them for consistency.- {name: 'buttonTextColor', default: ''}, + {name: 'buttonTextColor', default: 'none'},
42-42: Confirm'none'as the intended fallback.Using
'none'may be semantically ambiguous for a text color—if it’s truly invalid or a placeholder. Consider using something like'#000000'or'#ffffff'if you want a failsafe color, or keep'none'if it’s an explicit design decision.Do you want me to propose a more descriptive fallback approach for text color?
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
58-65: Synchronizing button color and text color.Linking button color to a matching text color is a neat approach for theming consistency. However, watch out for accessibility (sufficient contrast) when pairing these colors.
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (3)
128-133: Adopt consistent spelling for color/colour.These lines mix "colour" in the test description with "color" in the code. Choose one spelling throughout the codebase to maintain clarity and consistency.
135-143: Use dedicated data attributes instead of 'title'.Selecting the element by
title="Black"may break if the title attribute changes. Prefer stable, semantically named data attributes (e.g.,data-color="black") to make maintenance and testing more robust.
145-153: Abstract the repeated color-change steps.You’re duplicating similar color-change logic across multiple tests (e.g., black, grey, etc.). Consider factoring this into a helper function that receives a color parameter and checks the resulting styles, improving maintainability.
📜 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(3 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(3 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(2 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
🔇 Additional comments (10)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
28-28: Constructor parameter validation.This new constructor parameter aligns with the overall approach, but ensure upstream calls consistently pass a valid color or
undefinedto correctly trigger the fallback.packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
59-59: Passing newbuttonTextColorprop.Great to see the new color prop passed consistently. Ensure all references, including styling logic, handle the fallback scenario.
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (3)
24-25: AddedbuttonTextColorprop to the component.This prop extension looks straightforward. Confirm that other usage sites (if any) also handle this prop as intended.
71-71: PropagatingbuttonTextColortoCtaCard.Good addition for ensuring text color is directly controlled. This should make the CTA card fully themeable.
74-74: WiringhandleButtonColorcallback.The callback pattern is consistent and keeps the parent’s state in sync.
packages/kg-default-nodes/test/nodes/call-to-action.test.js (3)
37-37: Default test dataset text color.Including
'none'here matches the constructor fallback, but consider updating the default in the property definition for total consistency.
64-64: Verifying extended property coverage.Adding an assertion ensures the new
buttonTextColorproperty is adequately tested.
99-102: Testing setter functionality.Excellent addition to ensure
buttonTextColormutates properly and remains consistent with the rest of the node’s state.packages/koenig-lexical/test/e2e/cards/cta-card.test.js (2)
155-162: Nice coverage of the color picker flow.This test does a good job verifying the end-to-end workflow for custom colors via the color picker, ensuring correct UI output without extraneous complexity.
164-176: Validate additional edge cases for text color changes.Currently, the test verifies correct text color for white and black backgrounds. It may be beneficial to test borderline scenarios (e.g., very light greys or invalid color entries) to ensure robust functionality.
ref https://linear.app/ghost/issue/PLG-316/implement-button-colour-handling-on-cta-card
Summary by CodeRabbit
New Features
Tests