Replaced visibility message with indicator icon #1429
Conversation
|
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/KoenigCardWrapper.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/CardWrapper.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/cards/HtmlCard.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.
WalkthroughThe pull request introduces enhancements to the content visibility management in the Koenig Lexical editor. The changes focus on refactoring how card visibility is handled, particularly around the Changes
Sequence DiagramsequenceDiagram
participant User
participant CardWrapper
participant KoenigCardWrapper
participant VisibilityIndicator
User->>CardWrapper: Click visibility indicator
CardWrapper->>KoenigCardWrapper: Trigger toggleEditMode
KoenigCardWrapper->>KoenigCardWrapper: Determine edit state
alt Edit mode on
KoenigCardWrapper->>CardWrapper: Show EDIT_CARD_COMMAND
else Edit mode off
KoenigCardWrapper->>CardWrapper: Show DESELECT_CARD_COMMAND
end
CardWrapper->>VisibilityIndicator: Update visibility state
Possibly related PRs
Poem
✨ 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: 0
🔭 Outside diff range comments (2)
packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx (1)
Line range hint
48-48: Fix incorrect PropType forcontentVisibility.The
contentVisibilityprop is defined asPropTypes.elementbut based on usage it should bePropTypes.bool.- contentVisibility: PropTypes.element, + contentVisibility: PropTypes.bool,packages/koenig-lexical/src/components/ui/CardWrapper.jsx (1)
Line range hint
109-119: Add PropTypes for new props.Several new props are being used but not declared in PropTypes:
featureisVisibilityActiveonIndicatorClickCardWrapper.propTypes = { isSelected: PropTypes.bool, isEditing: PropTypes.bool, cardWidth: PropTypes.oneOf(['regular', 'wide', 'full']), icon: PropTypes.string, indicatorPosition: PropTypes.shape({ left: PropTypes.string, top: PropTypes.string - }) + }), + feature: PropTypes.shape({ + contentVisibilityAlpha: PropTypes.bool, + contentVisibility: PropTypes.bool + }), + isVisibilityActive: PropTypes.bool, + onIndicatorClick: PropTypes.func };
🧹 Nitpick comments (2)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (2)
25-38: LGTM! Consider memoizing the editor.update callback.The toggleEditMode implementation is clean and handles both edit and deselect cases correctly.
Consider memoizing the editor.update callback to prevent unnecessary re-renders:
const toggleEditMode = React.useCallback((event) => { event.preventDefault(); event.stopPropagation(); + const updateCallback = React.useCallback(() => { + const cardNode = $getNodeByKey(nodeKey); + + if (cardNode?.hasEditMode?.() && !isEditing) { + editor.dispatchCommand(EDIT_CARD_COMMAND, {cardKey: nodeKey, focusEditor: true}); + } else if (isEditing) { + editor.dispatchCommand(DESELECT_CARD_COMMAND, {cardKey: nodeKey, focusEditor: true}); + } + }, [nodeKey, isEditing]); + - editor.update(() => { - const cardNode = $getNodeByKey(nodeKey); - - if (cardNode?.hasEditMode?.() && !isEditing) { - editor.dispatchCommand(EDIT_CARD_COMMAND, {cardKey: nodeKey, focusEditor: true}); - } else if (isEditing) { - editor.dispatchCommand(DESELECT_CARD_COMMAND, {cardKey: nodeKey, focusEditor: true}); - } - }); + editor.update(updateCallback); }, [editor, isEditing, nodeKey]);
147-153: Move isVisibilityActive computation to a useMemo hook.The visibility state computation should be memoized to prevent unnecessary recalculations on re-renders.
- let isVisibilityActive = false; - if (cardConfig?.feature?.contentVisibilityAlpha) { - editor.getEditorState().read(() => { - const cardNode = $getNodeByKey(nodeKey); - isVisibilityActive = cardNode?.getIsVisibilityActive?.(); - }); - } + const isVisibilityActive = React.useMemo(() => { + if (!cardConfig?.feature?.contentVisibilityAlpha) { + return false; + } + + let isActive = false; + editor.getEditorState().read(() => { + const cardNode = $getNodeByKey(nodeKey); + isActive = cardNode?.getIsVisibilityActive?.(); + }); + return isActive; + }, [editor, nodeKey, cardConfig?.feature?.contentVisibilityAlpha]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(4 hunks)packages/koenig-lexical/src/components/ui/CardWrapper.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx(2 hunks)packages/koenig-lexical/src/hooks/useVisibilityToggle.js(2 hunks)packages/koenig-lexical/src/nodes/HtmlNode.jsx(1 hunks)packages/koenig-lexical/src/utils/visibility.js(0 hunks)packages/koenig-lexical/test/e2e/content-visibility.test.js(1 hunks)packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js(1 hunks)packages/koenig-lexical/test/unit/utils/visibility.test.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/koenig-lexical/test/unit/utils/visibility.test.js
- packages/koenig-lexical/src/utils/visibility.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (10)
packages/koenig-lexical/src/hooks/useVisibilityToggle.js (2)
2-2: LGTM! Clean import statement.The removal of
generateVisibilityMessageAlphaimport aligns with the PR's objective to replace visibility messages with indicator icons.
21-22: LGTM! Visibility message logic update.The condition
!isContentVisibilityAlphaEnabledcorrectly ensures that visibility messages are only shown when the alpha feature is disabled, aligning with the PR's objective to use indicator icons instead.packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js (1)
108-109: LGTM! Test case updated to match new behavior.The test case correctly verifies that visibility messages are removed when contentVisibilityAlpha is enabled. The added comment provides clear documentation of the intentional change.
packages/koenig-lexical/src/nodes/HtmlNode.jsx (1)
34-34: Verify the implementation ofgetIsVisibilityActive().The code calls
this.getIsVisibilityActive()but this method is not visible in the current file. Please ensure that this method is properly implemented, either in this class or in the parentBaseHtmlNodeclass.✅ Verification successful
Implementation of
getIsVisibilityActive()is verified and correctThe method is properly implemented through the decorator node generator and inherited by the HtmlNode class. The implementation includes proper visibility property handling and is well-tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of getIsVisibilityActive ast-grep --pattern 'getIsVisibilityActive() { $$$ }' # Also check the base class implementation rg -A 5 "getIsVisibilityActive"Length of output: 4577
Script:
#!/bin/bash # Check HtmlNode's class definition and inheritance ast-grep --pattern 'class HtmlNode extends $_ { $$$ }' # Search for BaseHtmlNode or related class definitions rg -A 5 "class.*HtmlNode"Length of output: 6972
packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx (1)
11-14: LGTM! Clean implementation of feature flags.The extraction of feature flags and the logic for
displayVisibilityMessageis clear and well-structured.packages/koenig-lexical/src/components/ui/CardWrapper.jsx (1)
59-89: LGTM! Clean implementation of conditional indicator rendering.The logic for rendering either the visibility indicator or the card type indicator is well-structured and maintains consistent styling.
packages/koenig-lexical/test/e2e/content-visibility.test.js (4)
192-204: Well-structured test coverage for default visibility settings!The test comprehensively verifies the default state of all visibility toggles, ensuring content is visible across all platforms and member types by default.
205-224: Comprehensive test coverage for visibility toggles!The test methodically verifies each visibility toggle's functionality, and correctly documents the intentional removal of the visibility message as part of the new design.
226-241: Excellent test coverage for the new visibility indicator feature!The test effectively verifies both the visibility indicator's appearance conditions and its interactive behavior for toggling edit mode, aligning perfectly with the PR's objective of replacing the visibility message with an interactive indicator icon.
Line range hint
244-253: Good test coverage for Stripe-dependent visibility settings!The test effectively verifies that paid member visibility options are properly hidden when Stripe is not enabled, maintaining a consistent user experience.
ref https://linear.app/ghost/issue/PLG-330/ - removed visibility text when content visibility is active - added replacement of card type indicator icon with visibility icon when visibility settings are active - clicking toggles card in/out of edit mode for context-driven access to visibility settings
kevinansfield
force-pushed
the
visibility-indicator
branch
from
7db2938 to
5d54605
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (1)
147-153: Consider memoizing the visibility state calculation.The visibility state is recalculated on every render. Consider memoizing this value using
useMemoto optimize performance, especially since it involves a state read operation.- let isVisibilityActive = false; - if (cardConfig?.feature?.contentVisibilityAlpha) { - editor.getEditorState().read(() => { - const cardNode = $getNodeByKey(nodeKey); - isVisibilityActive = cardNode?.getIsVisibilityActive?.(); - }); - } + const isVisibilityActive = React.useMemo(() => { + if (!cardConfig?.feature?.contentVisibilityAlpha) { + return false; + } + let isActive = false; + editor.getEditorState().read(() => { + const cardNode = $getNodeByKey(nodeKey); + isActive = cardNode?.getIsVisibilityActive?.(); + }); + return isActive; + }, [cardConfig?.feature?.contentVisibilityAlpha, editor, nodeKey]);packages/koenig-lexical/test/e2e/content-visibility.test.js (2)
205-224: Consider adding error states testing.While the test covers the basic toggle functionality well, consider adding tests for error states and edge cases, such as:
- Rapid toggle clicks
- Toggle during card state transitions
- Network latency simulation
Line range hint
244-253: Consider adding more Stripe integration test cases.While the test verifies that paid member settings are hidden when Stripe is disabled, consider adding tests for:
- Stripe initialization failures
- Mid-session Stripe disconnection
- Stripe feature flag changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(4 hunks)packages/koenig-lexical/src/components/ui/CardWrapper.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx(2 hunks)packages/koenig-lexical/src/hooks/useVisibilityToggle.js(2 hunks)packages/koenig-lexical/src/nodes/HtmlNode.jsx(1 hunks)packages/koenig-lexical/src/utils/visibility.js(0 hunks)packages/koenig-lexical/test/e2e/content-visibility.test.js(1 hunks)packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js(1 hunks)packages/koenig-lexical/test/unit/utils/visibility.test.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/koenig-lexical/src/utils/visibility.js
- packages/koenig-lexical/test/unit/utils/visibility.test.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js
- packages/koenig-lexical/src/nodes/HtmlNode.jsx
- packages/koenig-lexical/src/hooks/useVisibilityToggle.js
- packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx
- packages/koenig-lexical/src/components/ui/CardWrapper.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (5)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (3)
2-2: LGTM! Import statements are correctly structured.The new imports for
KoenigComposerContextandDESELECT_CARD_COMMANDare properly organized and align with the component's enhanced functionality.Also applies to: 6-6
25-38: LGTM! Well-implemented toggle functionality.The
toggleEditModefunction is well-structured with proper event handling and clear logic for toggling between edit modes. Good use of React's useCallback for optimization.
171-171: LGTM! Props are properly passed to CardWrapper.The new props (
feature,isVisibilityActive,onIndicatorClick) are correctly passed to the CardWrapper component, maintaining a clean props interface.Also applies to: 176-176, 178-178
packages/koenig-lexical/test/e2e/content-visibility.test.js (2)
192-203: LGTM! Comprehensive default visibility test.The test thoroughly verifies all default visibility states for different member types and platforms.
226-241: LGTM! Well-structured visibility icon interaction test.The test effectively verifies both the visibility icon's appearance conditions and its edit mode toggle functionality.
ref https://linear.app/ghost/issue/PLG-330/