Added link button to CTA Card toolbar #1441
Conversation
ref https://linear.app/ghost/issue/PLG-329/add-ability-to-add-url-to-cta-card - Added a new href node property to the CTA Node - wired up the href node prop to Koenig-Lexcal - Implemented a add link Action Toolbar
WalkthroughThe changes introduce a new 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/koenig-lexical/src/nodes/CallToActionNode.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. packages/koenig-lexical/src/nodes/CallToActionNodeComponent.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. packages/koenig-lexical/test/e2e/cards/cta-card.test.jsOops! 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.
✨ 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 (
|
![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/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
119-125: Consider memoizing the reselectCTACard function.Since this function is used in callbacks, memoizing it with useCallback would prevent unnecessary recreations.
- const reselectCTACard = () => { + const reselectCTACard = React.useCallback(() => { editor.update(() => { const nodeSelection = $createNodeSelection(); nodeSelection.add(nodeKey); $setSelection(nodeSelection); }); - }; + }, [editor, nodeKey]);
132-137: Add URL validation to setHref function.Consider validating URLs before updating the href property to ensure only valid URLs are stored.
- const setHref = (newHref) => { + const setHref = (newHref) => { + // Skip empty URLs + if (!newHref) { + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.href = ''; + }); + return; + } + + try { + // Validate URL + new URL(newHref); + editor.update(() => { + const node = $getNodeByKey(nodeKey); + node.href = newHref; + }); + } catch (e) { + // Handle invalid URLs + console.warn('Invalid URL:', newHref); + } };packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
119-121: Add more test cases for href property.Consider adding the following test cases:
- Test href validation with invalid URLs
- Test href rendering in DOM export
+ it('validates href URLs', editorTest(function () { + const callToActionNode = new CallToActionNode(); + + // Valid URLs should be accepted + callToActionNode.href = 'https://example.com'; + callToActionNode.href.should.equal('https://example.com'); + + // Invalid URLs should be rejected or sanitized + callToActionNode.href = 'not-a-url'; + callToActionNode.href.should.equal(''); + })); + + it('renders href in DOM export', editorTest(function () { + dataset.href = 'https://example.com'; + const callToActionNode = new CallToActionNode(dataset); + const {element} = callToActionNode.exportDOM(exportOptions); + + const html = element.outerHTML.toString(); + html.should.containEql('href="https://example.com"'); + }));packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
444-452: LGTM! Consider adding more test cases for comprehensive coverage.The test effectively verifies the presence and basic interaction of the Link button and input field in the CTA card toolbar.
Consider adding the following test cases to improve coverage:
- Verify that entering a URL in the link input updates the CTA card's href property.
- Verify that the link input preserves existing URLs when reopened.
- Verify URL validation (e.g., invalid URLs are handled appropriately).
Example test case:
test('can set and update CTA card link from toolbar', async function () { await focusEditor(page); await insertCard(page, {cardName: 'call-to-action'}); await page.keyboard.press('Escape'); // Set initial link await page.locator('[aria-label="Link"]').click(); await page.getByTestId('link-input').fill('https://example.com'); await page.keyboard.press('Enter'); // Verify link was set await expect(page.locator('[data-kg-card="call-to-action"]')) .toHaveAttribute('href', 'https://example.com'); // Verify link persists await page.locator('[aria-label="Link"]').click(); await expect(page.getByTestId('link-input')) .toHaveValue('https://example.com'); });
📜 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/CallToActionNode.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(5 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/cta-card.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
Learnt from: ronaldlangeveld
PR: TryGhost/Koenig#1437
File: packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx:0-0
Timestamp: 2025-02-06T04:48:22.660Z
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.
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-02-06T04:48:22.660Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
19-20: LGTM!The
hrefproperty is correctly defined with URL type validation, which is a good practice for link properties.packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
90-90: LGTM!The
hrefprop is correctly passed through to the CallToActionNodeComponent.
ronaldlangeveld
changed the title
- Fixed a bug where the settings panel would open when using the toolbar features, like Link and Snippets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
36-36: Add input validation for href management.While the link management functions are well-implemented, consider adding URL validation to prevent invalid links:
const setHref = (newHref) => { + // Validate URL format + try { + if (newHref && !newHref.startsWith('#')) { + new URL(newHref); + } editor.update(() => { const node = $getNodeByKey(nodeKey); node.href = newHref; }); + } catch (e) { + console.warn('Invalid URL format:', newHref); + return false; + } + return true; };Also applies to: 121-139
187-199: Enhance accessibility for link management UI.Consider adding accessibility improvements to the link management UI:
<ActionToolbar data-kg-card-toolbar="link" isVisible={showLink} + aria-label="Link management toolbar" > <LinkInput cancel={cancelLinkAndReselect} href={href} + aria-label="URL input field" update={(_href) => { setHref(_href); cancelLinkAndReselect(); }} /> </ActionToolbar> <ToolbarMenuItem icon="link" isActive={href || false} label="Link" + title="Add link (Ctrl+K)" + shortcut="Ctrl+K" onClick = {() => { setShowLink(true); }} />Also applies to: 203-203, 208-210
📜 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/CallToActionNode.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(5 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/cta-card.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js
- packages/koenig-lexical/src/nodes/CallToActionNode.jsx
- packages/koenig-lexical/test/e2e/cards/cta-card.test.js
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
🧰 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-02-06T04:48:22.660Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (2)
4-4: LGTM! Well-structured imports and prop additions.The new imports and href prop are correctly implemented to support the link functionality.
Also applies to: 7-7, 28-28
141-143: LGTM! Correctly follows established patterns.The useEffect implementation correctly follows the established pattern of only setting the main content editor as editable.
ref https://linear.app/ghost/issue/PLG-329/add-ability-to-add-url-to-cta-card