Moved HTML card web visibility settings to beta #1435
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/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/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/nodes/HtmlNodeComponent.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.
WalkthroughThis pull request refactors how visibility is managed in various components. The changes replace the specific check for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Card
participant Config
User->>Card: Render card
Card->>Config: Check cardConfig.feature.visibility
alt Visibility Active
Card->>Card: Render VisibilityIndicator
else
Card->>Card: Render default IndicatorIcon
end
sequenceDiagram
participant User
participant VisibilitySettings
participant ToggleHandler
User->>VisibilitySettings: Click toggle button
VisibilitySettings->>ToggleHandler: Invoke toggleVisibility
ToggleHandler-->>VisibilitySettings: State updated
VisibilitySettings->>User: Rerender toggle state
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![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 (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (1)
48-53: Remove unused onBlur prop and handler.The TODO comment indicates that the
onBlurprop is not used by theHtmlCardcomponent. This dead code should be removed.Apply this diff to remove the unused code:
- // TODO: this isn't used? <HtmlCard> does not have a prop for `onBlur` - const onBlur = (event) => { - if (event?.relatedTarget?.className !== 'kg-prose') { - editor.dispatchCommand(DESELECT_CARD_COMMAND, {cardKey: nodeKey}); - } - };Also remove the
onBlurprop from theHtmlCardcomponent usage:<HtmlCard darkMode={darkMode} html={html} isEditing={cardContext.isEditing} updateHtml={updateHtml} - onBlur={onBlur} />
🧹 Nitpick comments (1)
packages/koenig-lexical/src/hooks/useVisibilityToggle.js (1)
15-28: Prevent potential undefined references.If
grouportoggleis not found during.find(...), this code could raise an error. Consider adding a check or fallback to handle the scenario of unknown group or toggle keys gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(1 hunks)packages/koenig-lexical/src/components/ui/CardWrapper.jsx(1 hunks)packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx(2 hunks)packages/koenig-lexical/src/hooks/useVisibilityToggle.js(1 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(3 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(0 hunks)
💤 Files with no reviewable changes (2)
- packages/koenig-lexical/src/utils/visibility.js
- packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (7)
packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx (1)
1-30: The new function-based approach is clear and maintainable.Implementation is well-structured. The code neatly maps each group and its toggles, provides unique keys, and correctly checks the index when deciding to render the divider. Overall, this streamlined design should be easy to maintain and extend.
packages/koenig-lexical/src/hooks/useVisibilityToggle.js (1)
2-2: Good usage of specialized helpers.Relying on
getVisibilityOptions,parseVisibilityToToggles, andserializeOptionsToVisibilitykeeps the code DRY and consistent.packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx (1)
7-43: Refactored prop structure is simpler.Removing the visibility messaging props reduces complexity. Passing
darkModethrough to theHtmlEditoris clean and consistent with the rest of the code.🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
packages/koenig-lexical/src/components/ui/CardWrapper.jsx (1)
60-60: LGTM! Simplified visibility indicator logic.The visibility indicator logic has been streamlined to only depend on
isVisibilityActive, which aligns with the transition from alpha to beta.packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (1)
21-29: LGTM! Clean transition to beta visibility settings.The visibility settings implementation has been simplified by:
- Using the standard VisibilitySettings component
- Streamlining the useVisibilityToggle hook usage
- Removing alpha-specific code
packages/koenig-lexical/test/e2e/content-visibility.test.js (1)
15-117: Verify test coverage for beta visibility features.While the removal of alpha-specific tests is appropriate, please ensure that the remaining tests cover all critical beta visibility features:
- Basic visibility toggle functionality
- Visibility indicator behavior
- Settings panel interactions
- Stripe integration checks
The current test suite covers these aspects, but consider adding tests for any new beta-specific behaviors.
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (1)
148-153: LGTM! Clean transition to beta feature flag.The feature flag check has been updated from
contentVisibilityAlphatocontentVisibilitywhile maintaining the same visibility checking logic.
closes https://linear.app/ghost/issue/PLG-339 - swapped `contentVisibilityAlpha` flag conditionals to `contentVisibility` where appropriate - deleted code left in place to support previous beta behaviour - added `ignoreCardSettings` option to our `assertHTML` test util and updated paste behaviour test so it's not dependent on changes that have no impact on what's being tested
kevinansfield
force-pushed
the
move-web-visibility-out-of-alpha
branch
from
869d970 to
8d90486
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (1)
48-53: Remove unused onBlur prop and handler.The TODO comment correctly identifies that the
onBlurhandler is unused. Since theHtmlCardcomponent doesn't accept anonBlurprop, this code can be safely removed.Apply this diff to remove the unused code:
- // TODO: this isn't used? <HtmlCard> does not have a prop for `onBlur` - const onBlur = (event) => { - if (event?.relatedTarget?.className !== 'kg-prose') { - editor.dispatchCommand(DESELECT_CARD_COMMAND, {cardKey: nodeKey}); - } - };And remove it from the HtmlCard props:
<HtmlCard darkMode={darkMode} html={html} isEditing={cardContext.isEditing} updateHtml={updateHtml} - onBlur={onBlur} />Also applies to: 62-62
🧹 Nitpick comments (1)
packages/koenig-lexical/test/e2e/content-visibility.test.js (1)
15-28: Consider adding more test coverage.While the current tests cover basic functionality, consider adding tests for:
- Edge cases when toggling multiple visibility options
- Persistence of visibility settings after card reload
- Interaction with other card settings
I can help generate additional test cases if you'd like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(1 hunks)packages/koenig-lexical/src/components/ui/CardWrapper.jsx(1 hunks)packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx(2 hunks)packages/koenig-lexical/src/hooks/useVisibilityToggle.js(1 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(3 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/e2e/paste-behaviour.test.js(1 hunks)packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js(0 hunks)packages/koenig-lexical/test/utils/e2e.js(5 hunks)
💤 Files with no reviewable changes (2)
- packages/koenig-lexical/src/utils/visibility.js
- packages/koenig-lexical/test/unit/hooks/useVisibilityToggle.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/koenig-lexical/src/components/KoenigCardWrapper.jsx
- packages/koenig-lexical/src/components/ui/CardWrapper.jsx
- packages/koenig-lexical/src/hooks/useVisibilityToggle.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (10)
packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx (1)
3-29: Clean and efficient implementation!The refactored
VisibilitySettingscomponent is well-structured with:
- Clear prop interface
- Efficient use of map functions
- Proper key usage in iterations
- Semantic class names
packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx (2)
7-7: Successfully removed visibility message functionality.The component interface has been simplified by removing visibility-related props, aligning with the PR objectives.
Also applies to: 39-44
20-23: Consider removing or documenting the empty z-index layer.The empty div with
z-50appears to serve no purpose in the current implementation.Could you clarify the purpose of this empty div with absolute positioning and z-index? If it's no longer needed, consider removing it:
- <div className="absolute inset-0 z-50 mt-0"></div>✅ Verification successful
I'll run a script to extract and review the relevant snippet from the file in question for more context.
Clarify or Remove the Redundant Overlay Element
Based on the context provided from the file at
packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx, the empty<div className="absolute inset-0 z-50 mt-0"></div>does not appear to have any active role (e.g. no event handling or visible content). If it was intended for a feature like an overlay or interaction blocker, its purpose should be clearly documented. Otherwise, it would be safe to remove it to simplify the code.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
Script:
#!/bin/bash # Display lines 15 to 40 of the file to see the context around the empty div. sed -n '15,40p' packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsxLength of output: 765
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (1)
21-29: Successfully simplified visibility management.The code cleanly handles visibility settings without alpha feature checks.
packages/koenig-lexical/test/e2e/content-visibility.test.js (1)
85-86: Test updated to reflect new visibility behavior.The comment and test assertion correctly document the removal of visibility messages as part of the beta changes.
packages/koenig-lexical/test/utils/e2e.js (3)
73-73: LGTM! Parameter addition follows existing pattern.The new
ignoreCardSettingsparameter is consistent with other ignore flags and aligns with the PR objectives.
126-142: LGTM! Well-structured changes with clear documentation.The changes are well-documented and follow the existing patterns:
- Clear comment explaining the order of operations
- Consistent use of data-testid selectors
150-160: LGTM! Logical reordering of operations.The reordering ensures that class and data-testid selectors are available for card-related replacements before being removed.
packages/koenig-lexical/test/e2e/paste-behaviour.test.js (2)
554-554: LGTM! Card UI enhancement.Added draggable div follows card UI requirements.
558-558: LGTM! Test adaptation for visibility settings.Added
ignoreCardSettings: truealigns with the PR objectives to move HTML card web visibility settings to beta.
1 participant
closes https://linear.app/ghost/issue/PLG-339
contentVisibilityAlphaflag conditionals tocontentVisibilitywhere appropriate