Wired up visibility settings for CTA card #1440
Conversation
WalkthroughThe pull request introduces several focused updates across components in the koenig-lexical package. In the UI components, modifications include updating dependency arrays in effect hooks, adding data attributes for testing, and restructuring JSX for enhanced clarity. The Possibly related PRs
Suggested reviewers
Poem
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/TabView.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/components/ui/SettingsPanel.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.
Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
closes https://linear.app/ghost/issue/PLG-332 - added `useVisibilityToggle` hook to `<CallToActionNodeComponent>` - passed `toggleVisibility` and `visibilityOptions` props from the hook through to `<CtaCard>` component ready for use by `<VisibilitySettings>` - replaced static visibility settings in `<CtaCard>` with `<VisibilitySettings>` - updated our `insertCard` e2e utility function to return a locator for the card making subsequent testing a little bit nicer
kevinansfield
force-pushed
the
cta-visibility-settings
branch
from
fc61c18 to
8ce0616
Compare
![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/components/ui/cards/CtaCard.jsx (2)
237-244: Consider improving the alt text for better accessibility.The current alt text "Placeholder" is generic. Consider making it more descriptive or dynamic based on the image content.
- alt="Placeholder" + alt={`CTA card ${layout} image`}
269-280: Consider enhancing button accessibility.The button could benefit from additional ARIA attributes for better screen reader support.
<Button color={'accent'} dataTestId="cta-button" placeholder="Add button text" + aria-label={buttonText || "Call to action button"} + role="link" + aria-haspopup="false" size={layout === 'immersive' ? 'medium' : 'small'} style={buttonColor ? { backgroundColor: buttonColor === 'accent' ? 'var(--accent-color)' : buttonColor, color: buttonTextColor } : undefined} value={buttonText} width={layout === 'immersive' ? 'full' : 'regular'} />packages/koenig-lexical/test/e2e/cards/cta-card.test.js (2)
369-402: Consider adding edge case tests for visibility settings.While the current test is thorough, consider adding tests for:
- Toggling all settings off and verifying the card's behavior
- Testing visibility inheritance from parent elements
- Testing visibility changes during card duplication
404-442: Consider adding error case tests for serialized visibility settings.The test covers the happy path well. Consider adding tests for:
- Invalid visibility settings in serialized data
- Missing visibility settings
- Malformed visibility data structure
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(1 hunks)packages/koenig-lexical/src/components/ui/TabView.jsx(2 hunks)packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(8 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(3 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(2 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(3 hunks)packages/koenig-lexical/test/utils/e2e.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx
- packages/koenig-lexical/src/components/ui/TabView.jsx
- packages/koenig-lexical/src/components/ui/SettingsPanel.jsx
- packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx
- packages/koenig-lexical/test/utils/e2e.js
- packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
11-11: LGTM! Visibility settings integration looks good.The new props and imports are correctly added to support visibility settings functionality.
Also applies to: 76-77, 86-87, 329-330
91-94: LGTM! Settings panel integration is well-structured.The visibility settings are cleanly integrated into a dedicated tab, following good separation of concerns.
Also applies to: 198-203, 296-299
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
19-32: LGTM! Test data setup is comprehensive.The serialized test card includes all necessary properties for thorough testing.
closes https://linear.app/ghost/issue/PLG-332
useVisibilityTogglehook to<CallToActionNodeComponent>toggleVisibilityandvisibilityOptionsprops from the hook through to<CtaCard>component ready for use by<VisibilitySettings><CtaCard>with<VisibilitySettings>insertCarde2e utility function to return a locator for the card making subsequent testing a little bit nicer