Updated card order in card menu #1445
Conversation
No ref - Updated the order to reflect frequency of use
WalkthroughThe pull request updates the 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/test/e2e/cards/email-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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ 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: 2
🧹 Nitpick comments (2)
packages/koenig-lexical/src/nodes/EmailNode.jsx (1)
1-1: Consider reviewing priority assignments for consistency.While most priority adjustments align with the goal of reflecting usage frequency, some changes (particularly HTML and Email nodes) seem to contradict this pattern. Consider:
- Lowering priorities for specialized features (HTML, Email)
- Maintaining higher priorities for commonly used features (Bookmark)
- Documenting the rationale for priority assignments to ensure maintainability
packages/koenig-lexical/src/nodes/ImageNode.jsx (1)
60-60: Consider adjusting GIF priority for consistency.The GIF option has a priority of 17, which seems high compared to related image options (Image: 1, Unsplash: 3). Consider lowering the priority to maintain a more logical grouping with other image-related options.
- priority: 17, + priority: 4,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/koenig-lexical/src/nodes/BookmarkNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/ButtonNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/CalloutNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/EmailCtaNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/EmailNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/GalleryNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/HeaderNode.jsx(2 hunks)packages/koenig-lexical/src/nodes/HorizontalRuleNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/HtmlNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/ImageNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/MarkdownNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/PaywallNode.jsx(1 hunks)packages/koenig-lexical/src/nodes/SignupNode.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/koenig-lexical/src/nodes/GalleryNode.jsx
🧰 Additional context used
🧠 Learnings (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (11)
packages/koenig-lexical/src/nodes/PaywallNode.jsx (1)
17-17: LGTM: Priority adjustment reflects usage patterns.The decrease in priority from 9 to 6 aligns with the goal of organizing cards based on frequency of use.
packages/koenig-lexical/src/nodes/HorizontalRuleNode.jsx (1)
17-17: LGTM: Divider prioritized appropriately.Moving the divider to priority 2 makes sense as it's a commonly used basic element for content organization.
packages/koenig-lexical/src/nodes/ButtonNode.jsx (1)
17-17: LGTM: Button accessibility improved.The priority increase from 10 to 3 improves accessibility for this commonly used interactive element.
packages/koenig-lexical/src/nodes/MarkdownNode.jsx (1)
18-18: LGTM: Markdown editor properly deprioritized.Moving markdown editing to priority 19 better reflects its specialized nature and less frequent usage compared to basic content elements.
packages/koenig-lexical/src/nodes/CalloutNode.jsx (1)
24-24: LGTM! Priority adjustment aligns with usage patterns.The moderate decrease in priority (11 → 9) seems appropriate for a feature that's commonly used for highlighting information but not as frequently as basic content types.
packages/koenig-lexical/src/nodes/BookmarkNode.jsx (1)
25-25: LGTM! Priority adjustment reflects common usage.The decrease in priority (6 → 4) appropriately positions the bookmark card higher in the menu, reflecting its frequent use for embedding links.
packages/koenig-lexical/src/nodes/EmailCtaNode.jsx (1)
25-25: LGTM! Priority change aligns with usage patterns.The priority adjustment from 8 to 7 aligns with the PR objective of updating card order based on frequency of use.
packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
24-25: LGTM! Enhanced discoverability with expanded matches.The changes improve the component in two ways:
- Priority adjustment aligns with EmailCtaNode, reflecting similar importance.
- Additional keywords in matches array make the CTA card more discoverable.
packages/koenig-lexical/src/nodes/SignupNode.jsx (1)
29-29: LGTM! Priority value appropriate for conversion component.The priority of 10 is well-chosen as it places the signup card after basic content components but before utility components, reflecting its importance in conversion while acknowledging it's not used as frequently as core content cards.
packages/koenig-lexical/src/nodes/HeaderNode.jsx (2)
31-31: LGTM! Priority adjustment for Header1 (v1) menu entry.The priority change from 17 to 11 aligns with the PR objective of reordering cards based on usage frequency.
46-46: LGTM! Priority adjustment for Header (v2) menu entry.The priority change from 17 to 11 aligns with the PR objective of reordering cards based on usage frequency. Having the same priority as Header1 ensures both header options appear together in the menu.
| @@ -15,7 +15,7 @@ export class HtmlNode extends BaseHtmlNode { | |||
| Icon: HtmlCardIcon, | |||
| insertCommand: INSERT_HTML_COMMAND, | |||
| matches: ['html'], | |||
| priority: 3, | |||
| priority: 18, | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider lowering the priority value for HTML card.
The significant increase in priority (3 → 18) seems to contradict the PR's objective of ordering cards by frequency of use. HTML editing is typically a more specialized feature used less frequently than basic content types.
Let's analyze the usage patterns to validate this assumption:
🏁 Script executed:
#!/bin/bash
# Search for HTML card usage patterns in the codebase
echo "Searching for HTML card usage..."
rg -l "HtmlNode|INSERT_HTML_COMMAND" --type js --type jsx
# Search for other card usages for comparison
echo -e "\nSearching for basic card usages..."
rg -l "ButtonNode|ImageNode|HeaderNode" --type js --type jsxLength of output: 338
I'll generate a revised script that explicitly targets .js and .jsx files using file globs rather than relying on file type detection. This should help us gather the actual usage data for HTML card features as compared to basic card usages.
Please run this revised script so we can verify how frequently the HTML card is used relative to other cards.
🏁 Script executed:
#!/bin/bash
# Searching for HTML card usage patterns with file glob for .js and .jsx files
echo "Searching for HTML card usage..."
rg -l "HtmlNode|INSERT_HTML_COMMAND" -g "*.js" -g "*.jsx"
# Searching for basic card usages with file glob for .js and .jsx files
echo -e "\nSearching for basic card usages..."
rg -l "ButtonNode|ImageNode|HeaderNode" -g "*.js" -g "*.jsx"Length of output: 2694
HTML Card Priority Mismatch: Lower Priority Value Recommended
- The usage search shows that HTML card–related code (e.g.
HtmlNode) appears in far fewer files than basic card types (e.g.ButtonNode,ImageNode,HeaderNode), supporting the notion that HTML is less frequently used. - Given the PR’s objective to order cards by frequency of use, raising the priority from 3 to 18 for the HTML card is counterintuitive.
- It would be more appropriate to use a lower priority value for the HTML card so that frequently-used cards (like button, image, or header cards) take precedence.
| @@ -22,7 +22,7 @@ export class EmailNode extends BaseEmailNode { | |||
| Icon: EmailCardIcon, | |||
| insertCommand: INSERT_EMAIL_COMMAND, | |||
| matches: ['email content', 'only email'], | |||
| priority: 7, | |||
| priority: 8, | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider maintaining or lowering the priority for email content.
The increase in priority (7 → 8) for email-specific content seems inconsistent with the goal of ordering by frequency of use, as this is a specialized feature typically used less often than general content types.
Let's analyze the usage patterns:
🏁 Script executed:
#!/bin/bash
# Search for email content usage patterns
echo "Searching for email content usage..."
rg -l "EmailNode|INSERT_EMAIL_COMMAND" --type js --type jsx
# Search for general content types for comparison
echo -e "\nSearching for general content type usage..."
rg -l "ButtonNode|ImageNode|HeaderNode" --type js --type jsxLength of output: 366
I'll now run an updated shell script that restricts the search to *.jsx files using the -g "*.jsx" flag:
🏁 Script executed:
#!/bin/bash
echo "Searching for email content usage..."
rg -l "EmailNode|INSERT_EMAIL_COMMAND" -g "*.jsx"
echo -e "\nSearching for general content type usage..."
rg -l "ButtonNode|ImageNode|HeaderNode" -g "*.jsx"Length of output: 1188
Action Required: Reevaluate the EmailNode Priority Increment
The evidence shows that email-specific components (referenced in 3 files) are considerably less frequent than general content types (referenced in 13 files). Given that email content is a specialized feature, increasing its priority from 7 to 8 may inadvertently elevate its ordering over more commonly used content. Please consider maintaining the original priority or lowering it to better reflect its lower usage frequency.
No ref
No ref