-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Updated card order in card menu #1445
base: main
Are you sure you want to change the base?
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/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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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: 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.
No ref
Ref https://linear.app/ghost/issue/PLG-356/update-card-order-in-card-menu - Added explicit editor focus before typing command - Implemented race condition handling for menu/item visibility - Added fallback retry mechanism with Escape key - Increased selector timeouts to 5000ms
No ref