-
-
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
Fixed call-to-action card in storybook #1448
Conversation
Ref https://linear.app/ghost/issue/PLG-351/standardise-naming-convention-for-call-to-action-components - Import was broken due to file name change - Editable sponsor label broke the call-to-action card in storybook
WalkthroughThe changes update the story configuration for a card component by renaming it from 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/components/ui/cards/CallToActionCard.stories.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. ✨ 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 (
|
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 (3)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.stories.jsx (3)
69-72
: Consider making the sponsor label text configurable.The sponsor label text is currently hardcoded as 'Sponsored'. Consider making it configurable through story args to support different use cases and localization.
- initialHtml: 'Sponsored' + initialHtml: args.sponsorLabelText || 'Sponsored'Don't forget to add the new arg to the story configuration:
argTypes: { // ... existing args sponsorLabelText: { control: 'text', description: 'Text to display in the sponsor label' } }
63-73
: Consider extracting editor initialization logic.The editor initialization logic could be extracted to a helper function to improve code organization and reusability.
const initializeEditor = (nodes, initialHtml) => { const editor = createEditor({nodes}); return { editor, editorState: populateEditor({ editor, initialHtml }) }; };Then use it like this:
- let sponsorLabelHtmlEditor = null; - let sponsorLabelHtmlEditorInitialState = null; - - if (args.hasSponsorLabel) { - sponsorLabelHtmlEditor = createEditor({nodes: BASIC_NODES}); - sponsorLabelHtmlEditorInitialState = populateEditor({ - editor: sponsorLabelHtmlEditor, - initialHtml: 'Sponsored' - }); - } + const { + editor: sponsorLabelHtmlEditor, + editorState: sponsorLabelHtmlEditorInitialState + } = args.hasSponsorLabel + ? initializeEditor(BASIC_NODES, args.sponsorLabelText || 'Sponsored') + : { editor: null, editorState: null };
99-127
: Consider adding more story variations.While the current stories cover basic empty and populated states, consider adding stories that test edge cases:
- Very long sponsor labels
- Different button colors with various background colors
- Different layouts with and without sponsor labels
Example additional story:
export const LongSponsorLabel = Template.bind({}); LongSponsorLabel.args = { ...Populated.args, sponsorLabelText: 'This is a very long sponsor label that tests text wrapping' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.stories.jsx
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/koenig-lexical/src/components/ui/cards/CallToActionCard.stories.jsx (2)
5-6
: LGTM! Component renaming aligns with standardization goals.The renaming from
CtaCard
toCallToActionCard
follows a more descriptive naming convention, making the code more maintainable and self-documenting.
21-22
: LGTM! Story configuration updated consistently.The story title and component reference have been updated to match the new component name, maintaining consistency throughout the codebase.
Ref https://linear.app/ghost/issue/PLG-351/standardise-naming-convention-for-call-to-action-components