Added alignment setting to Call To Action Card #1488
Conversation
Ref https://linear.app/ghost/issue/PLG-379/add-center-alignment-as-a-setting - To support generic, center-aligned call-to-actions, we've added an option to center align content when the full-width layout is selected. - As part of this, the button is now only full-width in the immersive layout when there is an image.
WalkthroughThis pull request introduces a new Possibly related PRs
Suggested reviewers
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. Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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 (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
243-252: Simplify unnecessary Fragment.The alignment settings are correctly shown only when the layout is 'immersive', which aligns with the PR requirements. However, there's an unnecessary Fragment wrapper that can be simplified.
{layout === 'immersive' && - <> - <ButtonGroupSettingBeta - buttons={alignmentOptions} - label='Alignment' - selectedName={alignment} - onClick={updateAlignment} - /> - </> + <ButtonGroupSettingBeta + buttons={alignmentOptions} + label='Alignment' + selectedName={alignment} + onClick={updateAlignment} + /> }🧰 Tools
🪛 Biome (1.9.4)
[error] 244-251: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
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(4 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(2 hunks)packages/koenig-lexical/src/components/ui/TabView.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(9 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(4 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
Learnt from: ronaldlangeveld
PR: TryGhost/Koenig#1437
File: packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx:0-0
Timestamp: 2025-03-17T01:06:14.089Z
Learning: In the CallToActionNodeComponent of Koenig, only the main content editor (htmlEditor) should be set as editable in the useEffect hook. The sponsor label editor's (sponsorLabelHtmlEditor) editability is handled separately as it's an optional feature.
🧬 Code Definitions (1)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
packages/koenig-lexical/test/utils/e2e.js (2) (2)
focusEditor(58:61)insertCard(404:417)
🪛 Biome (1.9.4)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx
[error] 244-251: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (28)
packages/koenig-lexical/src/components/ui/TabView.jsx (1)
13-13: Spacing adjustment for better tab presentation.The increased gap from
gap-3togap-4provides more breathing room between tabs, which helps with readability and is more consistent with common UI patterns.packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
11-11: Property addition for alignment enhancement.The new
alignmentproperty with default value of'left'is a good foundation for the CTA component's alignment options. The default value ensures backward compatibility with existing cards.Do other alignment values like 'center' or 'right' need to be documented or validated anywhere in the code? Consider adding validation if there are only specific allowed values.
packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
99-99: Node property properly passed to component.Good implementation of passing the alignment property to the CallToActionNodeComponent, which ensures the UI can utilize the alignment setting.
Is there a corresponding handler function like
handleUpdatingAlignmentin the CallToActionNodeComponent to allow users to change this value? Make sure the two-way binding is implemented.packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-parser.js (2)
13-13: DOM attribute extraction for alignment.The parser correctly extracts the alignment value from
data-alignmentattribute with a sensible default of 'left'.
48-48:✅ Verification successful
Payload inclusion of alignment property.
Proper inclusion of the alignment property in the payload object ensures the alignment setting is preserved when parsing HTML.
🏁 Script executed:
#!/bin/bash # Check if there's a corresponding renderer file that needs to use this alignment property find packages -name "*calltoaction-renderer*" -type f | xargs grep -l "alignment" || echo "No renderer files found using alignment"Length of output: 157
Alignment Payload Inclusion Confirmed
The alignment property is correctly included in the payload, and verification shows that the corresponding renderer file at
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.jsutilizes this property. No further action is required.packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (3)
16-16: LGTM: Added new alignment prop.The new
alignmentprop correctly extends the component's interface to support the center alignment feature mentioned in the PR objectives.
137-142: LGTM: Well-implemented alignment handler.The
handleUpdatingAlignmentfunction follows the established pattern for property updates in this component, maintaining consistency with other handler functions.
155-155: LGTM: Properly passed alignment props to CallToActionCard.The alignment property and update handler are correctly passed to the child component, providing a complete implementation of the new feature.
Also applies to: 180-180
packages/kg-default-nodes/test/nodes/call-to-action.test.js (2)
162-162: LGTM: Added default alignment to dataset tests.The default alignment value is correctly added to the dataset in the
getDataset()method test, ensuring proper test coverage for the new property.
504-504: LGTM: Added alignment to JSON export tests.The alignment property is correctly included in the JSON export test, ensuring the serialization of the new property works as expected.
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (4)
28-28: LGTM: Added conditional class for centered alignment.The CSS class
kg-cta-centeredis correctly added conditionally based on the alignment property value, implementing the visual aspect of the alignment feature.
160-160: Verify button table width behavior in email clients.The
width="100%"attribute has been removed from the table element that wraps the button in the email template. This might affect how the button displays in various email clients.Could you test this change in various email clients to ensure the button displays correctly without the width attribute? This is particularly important for ensuring consistent rendering across different email clients.
182-182: LGTM: Added alignment class to email template.The alignment class is correctly applied to the email template as well, ensuring consistent styling across web and email rendering.
206-206: LGTM: Added alignment to renderer dataset.The alignment property is properly included in the dataset object used by the renderer, making it available throughout the rendering process.
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (3)
366-374: LGTM: Added test for hiding alignment settings in minimal layout.This test correctly verifies that alignment settings are not visible when using the minimal layout, as specified in the PR objectives.
376-390: LGTM: Added test for visibility of alignment settings in immersive layout.This test comprehensively verifies that alignment settings become visible when switching to immersive layout and become hidden again when returning to minimal layout.
392-411: LGTM: Added test for alignment functionality in immersive layout.This test thoroughly validates the alignment feature by checking that:
- Left alignment is the default
- Content editor class changes when switching to center alignment
- Content editor class reverts when switching back to left alignment
The test provides good coverage of the core alignment functionality.
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (11)
1-1: Good addition of necessary import components.The imports for
CenterAlignIconandLeftAlignIconare well-placed at the top of the file and are needed for the new alignment feature.Also applies to: 4-4
98-99: Appropriate default props and handlers implementation.The new
alignmentprop with a default value of 'left' and the correspondingupdateAlignmenthandler function follow the component's existing patterns for props and handlers. This ensures backward compatibility with existing usages of the component.Also applies to: 120-121
147-155: Clear UI text improvements in layout options.Changing the label from 'Immersive' to 'Full' makes the UI more intuitive, and simplifying the aria-labels improves accessibility by making them more concise and descriptive.
158-171: Well-structured alignment options configuration.The new
alignmentOptionsarray follows the same pattern as thelayoutOptionsarray, providing consistent styling and behavior for both settings. The data-testid attributes will also facilitate automated testing of the new alignment feature.
368-371: Good implementation of flex alignment based on selected options.The conditional CSS class application for center alignment is well implemented. When layout is 'immersive' and alignment is 'center', the 'items-center' class is applied to center the flex items within the container.
383-386: Proper text alignment implementation for the content editor.The conditional class application for text alignment ensures that the text is properly aligned according to the selected alignment option when in 'immersive' layout. The use of 'text-pretty' also enhances text rendering, and the existing classes maintain the component's styling.
393-395: Improved button container width handling.The button container now correctly adjusts its width based on the layout type and presence of an image, ensuring the button displays at full width in the immersive layout with an image as required.
401-402: Proper button size adaptation based on layout.The button size is now dynamically set based on the layout and presence of an image, which ensures consistent visual appearance across different configurations of the call-to-action card.
407-408: Correct button width implementation.The button width is now properly set to 'full' when in immersive layout with an image present, or 'regular' otherwise. This ensures the button adheres to the design requirements specified in the PR objectives.
436-437: Appropriate PropTypes definitions for new properties.The PropTypes for the new
alignmentprop andupdateAlignmentfunction are correctly defined, ensuring proper type checking and documentation of the component's API.Also applies to: 449-450
1-470:❓ Verification inconclusive
Verify alignment interactions with theme and existing styles.
The implementation aligns well with the PR objectives to add center alignment for call-to-action cards in full-width layout. The conditional rendering and styling are appropriate, but it would be good to verify how this behaves with different themes and color combinations.
🌐 Web query:
What are best practices for implementing text alignment options in React components?💡 Result:
To implement text alignment effectively in React components, follow these best practices combining technical implementation with UI/UX principles:
Core Alignment Techniques
Use CSS-in-JS or Utility Classes
Apply alignment via:// Inline style (avoid for large components) <div style={{ textAlign: 'left' }}> // Tailwind CSS <p className="text-center md:text-left">Tailwind's responsive utilities ([8][14]) simplify cross-device alignment.
Native CSS Properties
Leverage standard CSS properties liketext-alignandvertical-align([5][11][14]):/* Block-level alignment */ .justified-text { text-align: justify; text-align-last: center; }React-Specific Implementation
Component Patterns
Use compound components for complex alignment scenarios:<TextBlock align="right"> <Header /> <Body /> </TextBlock>This avoids prop drilling ([7][9]).
Styled Components
Implement dynamic alignment with transient props ([9]):const Paragraph = styled.p` text-align: ${({ $align }) => $align || 'left'}; `; // Usage <Paragraph $align="justify">UI/UX Considerations
Readability First
- Left-align body text longer than 3 lines ([4][6])
- Right-align numbers in tables/dashboards ([4][13])
- Center-align short headlines (<2 lines) ([4][6])
Responsive Design
// Media queries in styled-components const ResponsiveText = styled.div` text-align: left; @media (min-width: 768px) { text-align: ${({ desktopAlign }) => desktopAlign}; } `;Use relative units (
rem,%) instead of fixed pixels ([3][6]).Accessibility
- Maintain 4.5:1 contrast ratio for aligned text
- Ensure proper heading hierarchy when aligning titles ([6])
Cross-Platform Consistency
For React Native:
<TextInput multiline={true} textAlignVertical="top" // Android style={{ paddingVertical: 0 }} // iOS />Combine platform-specific properties for unified behavior ([1][15]).
Testing & Maintenance
Visual Regression Testing
Use tools like Repeato ([1]) or Storybook to verify alignment across:
- Screen sizes
- RTL languages
- Dynamic content
Style Organization
// components/Text/styles.js export const ALIGNMENTS = { left: 'text-left', center: 'text-center', right: 'text-right', justify: 'text-justify' };Centralize alignment definitions for easier maintenance ([7][9]).
When implementing rich text editors, ensure alignment applies to entire blocks rather than inline selections ([2]). For data grids, use dedicated alignment classes like
htCenter([13]) to maintain table readability. Always validate alignment choices through user testing, particularly for long-form content and complex layouts.Citations:
- 1: https://www.repeato.app/aligning-text-input-correctly-in-react-native/
- 2: https://ej2.syncfusion.com/react/documentation/rich-text-editor/text-alignments
- 3: https://www.freecodecamp.org/news/how-to-style-react-components/
- 4: https://blog.prototypr.io/text-alignment-best-practises-c4114daf1a9b
- 5: https://www.w3schools.com/css/css_text_align.asp
- 6: https://www.uxpin.com/studio/blog/alignment-in-design-making-text-and-visuals-more-appealing/
- 7: https://www.freecodecamp.org/news/best-practices-for-react/
- 8: https://tailwindcss.com/docs/text-align
- 9: https://www.robinwieruch.de/styled-components/
- 10: react-native-elements/react-native-elements#2087
- 11: https://developer.mozilla.org/en-US/docs/Web/CSS/text-align
- 12: react-bootstrap/react-bootstrap#1586
- 13: https://handsontable.com/docs/react-data-grid/text-alignment/
- 14: https://www.w3schools.com/cssref/pr_text_text-align.php
- 15: https://reactnative.dev/docs/text-style-props
Attention: Verify Alignment Interactions Across Themes
The conditional text alignment implementation (using
'text-center'for immersive layouts with center alignment and defaulting to'text-left') follows React best practices and leverages Tailwind utility classes appropriately. However, please manually verify that these alignment choices interact as expected with different theme styles and color configurations—especially regarding dynamic accent colors and readability in various themes.
- Confirm that immersive layouts consistently render center-aligned text under multiple theme settings.
- Test color combinations to ensure the inline style (
--cta-link-color) and utility classes maintain clear, accessible contrast.🧰 Tools
🪛 Biome (1.9.4)
[error] 244-251: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
Ref https://linear.app/ghost/issue/PLG-379/add-center-alignment-as-a-setting - When the CTA card is center-aligned and the editor is empty, the cursor should be positioned at the beginning of the placeholder text rather than in the center.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (1)
243-252: Remove unnecessary Fragment wrapper.The Fragment (
<></>) is redundant as it contains only one child element.{layout === 'immersive' && - <> <ButtonGroupSettingBeta buttons={alignmentOptions} label='Alignment' selectedName={alignment} onClick={updateAlignment} /> - </> }🧰 Tools
🪛 Biome (1.9.4)
[error] 244-251: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(9 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx
[error] 244-251: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (9)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (9)
1-1: Appropriate icon imports for the new alignment feature.Both the center and left align icons are correctly imported to support the new alignment options.
Also applies to: 4-4
98-98: Alignment parameter properly implemented with appropriate defaults.The alignment parameter is added with a sensible default of 'left', and the updateAlignment function is provided with a default implementation. This maintains backward compatibility while enabling the new feature.
Also applies to: 120-120
147-147: Improved layout option labels for better UX.Changing "Immersive" to "Full" and simplifying the aria-labels makes the UI more intuitive and accessible.
Also applies to: 150-150, 154-155
158-170: Well-structured alignment options implementation.The alignment options array is well-organized and follows the same pattern as layoutOptions, with appropriate icons, labels, and test IDs for each option.
368-371: Proper implementation of center alignment styling.The flex container now correctly applies center alignment when appropriate, allowing the content to be visually centered in the immersive layout as specified in the requirements.
383-386: Text alignment properly handled with special case for placeholders.The text alignment classes are conditionally applied based on the selected alignment and layout. The special handling for placeholders ([&:has(.placeholder)]) ensures they remain properly aligned even in centered mode, which is a nice UX touch.
393-395: Button container width properly adjusted based on layout.The button container width is now correctly set to full-width only when in immersive layout with an image present, as specified in the requirements.
397-402: Button size and width appropriately adjusted for different layouts.The button now adapts its size and width based on the layout and presence of an image, creating a more consistent and visually pleasing appearance across different configurations.
Also applies to: 407-407
436-436: PropTypes properly defined for new alignment functionality.The PropTypes for alignment and updateAlignment are correctly defined, documenting the component's API and providing type safety.
Also applies to: 449-449
2 participants
Ref https://linear.app/ghost/issue/PLG-379/add-center-alignment-as-a-setting