-
-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added CallToAction Lexical Nodes #1412
Conversation
30ebfe2
to
00a9e55
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1412 +/- ##
==========================================
+ Coverage 90.81% 90.82% +0.01%
==========================================
Files 184 185 +1
Lines 17164 17218 +54
Branches 1899 1911 +12
==========================================
+ Hits 15587 15639 +52
- Misses 1567 1569 +2
Partials 10 10 ☔ View full report in Codecov by Sentry. |
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/kg-default-nodes.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 Developer
participant CallToActionNode
participant Editor
Developer->>CallToActionNode: Create with dataset
CallToActionNode-->>Developer: Node instance
Developer->>Editor: Add node to editor
Editor->>CallToActionNode: Configure properties
CallToActionNode-->>Editor: Update confirmed
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
19-42
: Improve default value handling in constructor.Consider these improvements:
- Use nullish coalescing (
??
) instead of logical OR (||
) to handle falsy values correctly- Use object destructuring with defaults to simplify the code
Here's the suggested implementation:
constructor({ - layout, - textValue, - showButton, - buttonText, - buttonUrl, - hasSponsorLabel, - hasBackground, - backgroundColor, - hasImage, - imageUrl - } = {}, key) { + layout = 'immersive', + textValue = '', + showButton = false, + buttonText = '', + buttonUrl = '', + hasSponsorLabel = false, + hasBackground = false, + backgroundColor = '#123456', + hasImage = false, + imageUrl = '' + } = {}, key) { super(key); - this.__layout = layout || 'immersive'; - this.__textValue = textValue || ''; - this.__showButton = showButton || false; - this.__buttonText = buttonText || ''; - this.__buttonUrl = buttonUrl || ''; - this.__hasSponsorLabel = hasSponsorLabel || false; - this.__hasBackground = hasBackground || false; - this.__backgroundColor = backgroundColor || '#123456'; - this.__hasImage = hasImage || false; - this.__imageUrl = imageUrl || ''; + this.__layout = layout; + this.__textValue = textValue; + this.__showButton = showButton; + this.__buttonText = buttonText; + this.__buttonUrl = buttonUrl; + this.__hasSponsorLabel = hasSponsorLabel; + this.__hasBackground = hasBackground; + this.__backgroundColor = backgroundColor; + this.__hasImage = hasImage; + this.__imageUrl = imageUrl; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/kg-default-nodes/lib/kg-default-nodes.js
(3 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js
(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (7)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (3)
4-16
: Consider adding URL validation for button and image URLs.The
buttonUrl
andimageUrl
properties should include URL validation to ensure they contain valid URLs.Also, consider making the default background color configurable rather than hardcoding it to '#123456'.
45-47
: LGTM!The factory function implementation is clean and follows the established pattern.
49-51
: LGTM!The type check implementation is correct and consistent with other nodes.
packages/kg-default-nodes/lib/kg-default-nodes.js (2)
7-7
: LGTM!Import statement follows the established pattern.
58-58
: LGTM!Export and registration in DEFAULT_NODES are correctly implemented.
Also applies to: 94-94
packages/kg-default-nodes/test/nodes/call-to-action.test.js (2)
135-137
: Implement missing functionality.Several key features are marked as "not yet implemented":
- urlTransformMap
- exportDOM/JSON
- importJSON/DOM
- getTextContent
These features are typically required for proper node serialization and content extraction. Please implement them to ensure full functionality.
Would you like me to help implement these features or create issues to track them?
Also applies to: 146-174
9-116
: LGTM! Well-structured test implementation.The test suite is well-organized with:
- Proper error handling
- Comprehensive property testing
- Good test setup practices
ref PLG-310
Summary by CodeRabbit
New Features
Tests