Added visibility rendering to Call to Action card #1436
Conversation
ref https://linear.app/ghost/issue/PLG-332 - extracted visibility rendering logic and adapted to be more generalised for use in other cards
ref https://linear.app/ghost/issue/PLG-332 - used visibility render method on CTA card's render output to wrap contents where necessary
|
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.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'packages/kg-default-nodes/.eslintrc.js': Cannot find module '@babel/eslint-parser'
packages/kg-default-nodes/lib/nodes/html/html-renderer.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'
packages/kg-default-nodes/lib/utils/visibility.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'
WalkthroughThis update refactors rendering logic for call-to-action and HTML nodes by centralizing visibility handling through a newly introduced Changes
Sequence Diagram(s)sequenceDiagram
participant CTA as CallToActionNode
participant RCTA as renderCallToActionNode
participant RWV as renderWithVisibility
participant Util as Visibility Helpers
CTA->>RCTA: Invoke renderCallToActionNode(node, options)
RCTA->>RWV: Pass output and visibility settings
RWV->>Util: Evaluate visibility (email or web)
Util-->>RWV: Return modified content based on visibility
RWV-->>RCTA: Return rendered output
RCTA-->>CTA: Send final rendering
sequenceDiagram
participant HTML as HtmlNode
participant RHTML as renderHtmlNode
participant RWV as renderWithVisibility
participant Util as Visibility Helpers
HTML->>RHTML: Invoke renderHtmlNode(node, options)
RHTML->>RWV: Delegate visibility handling if required
RWV->>Util: Process based on email/web conditions
Util-->>RWV: Return updated render output
RWV-->>RHTML: Return final content
RHTML-->>HTML: Deliver rendered HTML
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
kevinansfield
changed the title
![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 (4)
packages/kg-default-nodes/lib/utils/visibility.js (2)
81-92: Add error handling for invalid render types.The
_getRenderContentfunction should handle invalidtypevalues to prevent potential runtime errors.function _getRenderContent({element, type}) { + if (!element) { + return ''; + } if (type === 'inner') { return element.innerHTML; } else if (type === 'value') { if ('value' in element) { return element.value; } return ''; } else { + // Default to outerHTML for unknown types return element.outerHTML; } }
40-77: Consider adding JSDoc comments for better documentation.The
renderWithVisibilityfunction is a key utility that would benefit from detailed documentation of its parameters and return value.+/** + * Renders content with visibility rules for web and email targets. + * @param {Object} originalRenderOutput - The original render output containing element and type. + * @param {Object} visibility - The visibility rules for web and email. + * @param {Object} options - Rendering options including target. + * @returns {Object} The rendered output with visibility rules applied. + */ export function renderWithVisibility(originalRenderOutput, visibility, options) {packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (1)
84-84: Add null checks before accessing firstElementChild.The code assumes
emailDivandelementwill always have afirstElementChild. Add null checks to handle edge cases gracefully.- return renderWithVisibility({element: emailDiv.firstElementChild}, node.visibility, options); + const firstChild = emailDiv.firstElementChild; + if (!firstChild) { + throw new Error('Failed to render CTA email template'); + } + return renderWithVisibility({element: firstChild}, node.visibility, options); - return renderWithVisibility({element: element.firstElementChild}, node.visibility, options); + const firstChild = element.firstElementChild; + if (!firstChild) { + throw new Error('Failed to render CTA template'); + } + return renderWithVisibility({element: firstChild}, node.visibility, options);Also applies to: 91-91
packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
259-283: Tests look good, but could be more descriptive and DRY.The test cases effectively verify visibility rendering for both web and email targets. However, consider these improvements:
- More descriptive test names:
-it('should render with web visibility' +it('should wrap content in gated block comments when rendering for web with visibility settings' -it('should render with email visibility' +it('should add segment data attributes when rendering for email with visibility settings'
- Extract common visibility setup:
+beforeEach(function () { + // ... existing setup ... + + // Common visibility setup + dataset.visibility = {...utils.visibility.buildDefaultVisibility()}; +}); it('should render with web visibility', editorTest(function () { exportOptions.target = 'web'; - dataset.visibility = {...utils.visibility.buildDefaultVisibility(), web: {nonMember: false, memberSegment: 'status:free,status:-free'}}; + dataset.visibility.web = {nonMember: false, memberSegment: 'status:free,status:-free'};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(3 hunks)packages/kg-default-nodes/lib/nodes/html/html-renderer.js(2 hunks)packages/kg-default-nodes/lib/utils/visibility.js(2 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(2 hunks)packages/kg-default-nodes/test/utils/visibility.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (3)
packages/kg-default-nodes/lib/nodes/html/html-renderer.js (1)
3-3: LGTM! Clean refactoring of visibility handling.The changes effectively delegate visibility handling to the
renderWithVisibilityutility function, making the code more maintainable and consistent with other renderers.Also applies to: 20-22
packages/kg-default-nodes/test/utils/visibility.test.js (1)
62-200: LGTM! Excellent test coverage.The test suite is comprehensive and well-structured, covering:
- Email and web targets
- Different render types
- Old and new visibility formats
- Edge cases and error conditions
packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
3-4: LGTM!Clean organization of imports improves readability.
ref https://linear.app/ghost/issue/PLG-332