Added visibility toggle to html card toolbar #1491
Conversation
Ref https://linear.app/ghost/issue/PLG-307/change-trigger-for-visibility-panel-on-html-card - We're changing the way the visibility panel is triggered on the html card; instead of opening upon inserting the card, it can now be triggered via an icon in the toolbar.
WalkthroughThe pull request introduces a new context hook, Possibly related PRs
Suggested reviewers
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/test/e2e/cards/html-card.test.jsOops! 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. packages/koenig-lexical/demo/DemoApp.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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Definitions (2)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (4)
packages/koenig-lexical/test/e2e/cards/html-card.test.js (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (13)
✨ Finishing Touches
🪧 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
ref https://linear.app/ghost/issue/PLG-307/change-trigger-for-visibility-panel-on-html-card - added an additional state in the HTML Node Component that would handle the state of the visibility panel. - added comprehensive tests to ensure the state remains consistant with the behaviour
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/koenig-lexical/test/e2e/cards/html-card.test.js (2)
201-201: Remove extra comment slashThere's a double comment marker on this line that should be cleaned up.
- // // Click the visibility button to show the panel + // Click the visibility button to show the panel
214-214: Incomplete comment at end of fileThis comment appears to be incomplete and should either be completed or removed.
- // we need t
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(3 hunks)packages/koenig-lexical/test/e2e/cards/html-card.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx
🧰 Additional context used
🧬 Code Definitions (1)
packages/koenig-lexical/test/e2e/cards/html-card.test.js (1)
packages/koenig-lexical/test/utils/e2e.js (1)
focusEditor(58-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/koenig-lexical/test/e2e/cards/html-card.test.js (2)
178-186: Clean and clear test for visibility panel default stateThis test correctly verifies that the visibility panel is not displayed by default when an HTML card is in edit mode, which aligns with the PR objective to change the trigger mechanism for the visibility panel.
188-212: Well-structured behavior test for visibility panel state managementThis test thoroughly verifies the behavior of the visibility panel when toggling between editing states. It confirms that:
- The panel is not visible by default in edit mode
- The panel becomes visible when the visibility button is clicked
- The panel is automatically hidden when editing is turned off
- The panel becomes visible again when clicking on the card after editing is disabled
The test captures all the key scenarios for the new visibility toggle feature.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx (1)
9-9: Consider a more descriptive variable name for clarity.
showVisibilitySettingscould optionally be renamed to something likeisVisibilityPanelOpento provide an immediate understanding of its boolean nature, making the code more self-explanatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(3 hunks)packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx(1 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(5 hunks)packages/koenig-lexical/test/e2e/cards/html-card.test.js(1 hunks)packages/koenig-lexical/test/e2e/content-visibility.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/test/e2e/cards/html-card.test.js
🧰 Additional context used
🧬 Code Definitions (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (7)
packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx (2)
useKoenigSelectedCardContext(35-35)useKoenigSelectedCardContext(35-35)packages/koenig-lexical/src/components/ui/ActionToolbar.jsx (1)
useKoenigSelectedCardContext(5-5)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (1)
React(100-100)packages/koenig-lexical/src/nodes/CodeBlockNodeComponent.jsx (1)
React(13-13)packages/koenig-lexical/src/nodes/EmailNodeComponent.jsx (2)
cardContext(13-13)cardContext(15-15)packages/koenig-lexical/src/nodes/ToggleNodeComponent.jsx (2)
cardContext(13-13)cardContext(15-15)packages/koenig-lexical/src/components/ui/ToolbarMenu.jsx (1)
ToolbarMenuItem(54-76)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (18)
packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx (1)
17-19: State additions look correct and consistent.
The inclusion ofshowVisibilitySettingsandsetShowVisibilitySettingsin the context value is properly handled. The memo dependencies are correctly updated to ensure the context re-renders only when these states change.Also applies to: 27-29
packages/koenig-lexical/test/e2e/content-visibility.test.js (6)
38-42: Test name aligns with the new behavior of hiding settings panel by default.
This clarifies expectations for the user interaction, ensuring that the visibility panel no longer appears automatically when the edit button is clicked. The assertion checkingnot.toBeVisible()is correct.
54-55: Toolbar interactions updated as intended.
Clicking the newly introduced"show-visibility"icon, followed by"tab-visibility", correctly reflects the new workflow for toggling the visibility settings panel.
93-94: Visibility toggling tested thoroughly.
Ensuring that clicking"show-visibility"and"tab-visibility"transitions to the correct panel and states is good for reliable UI coverage.
107-107: Further testing ensures consistent visibility interactions.
Indicating that Stripe is disabled and verifying the correct toggles are hidden helps maintain coverage for membership scenarios.
111-111: Verifies toggles remain hidden under correct conditions.
Nice job ensuring separate code paths for Stripe being disabled, preventing paid member toggles from incorrectly showing up.
118-131: New test "visibility indicator can toggle visibility settings panel" is comprehensive.
It checks multiple transitions—opening the panel, toggling members, focusing away, then re-opening from the indicator—for thorough coverage of the new display logic.packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (5)
12-12: Context import is appropriate.
UsinguseKoenigSelectedCardContextinside this component clearly indicates it will manage or consume card-specific states, keeping the design consistent with other Koenig context usage.
22-29: ResettingshowVisibilitySettingswhen editing ends is beneficial.
This useEffect reliably guards against leaving the panel open when the card is deselected. IncludingsetShowVisibilitySettingsin the dependency array ensures correct and timely updates.
66-69: Toggle function is concise.
toggleVisibilitySettingsneatly handles the open/close logic. Ensure that external triggers also consistently set the same state, preserving a single source of truth for the panel's visibility.
91-106: Visibility icon usage in the toolbar is well-implemented.
The addedToolbarMenuItemwithdataTestId="show-visibility"and a distinct icon fosters clarity in the UI. The onClick logic is straightforward, making it easy to maintain.
119-130: Conditional settings panel rendering is well-structured.
Conditionally renderingSettingsPanelonly ifisContentVisibilityEnabledandshowVisibilitySettingsare both true prevents accidental or unwanted display. Great approach to controlling UI complexity.packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (6)
20-20: Good addition of the visibility settings context.The addition of
setShowVisibilitySettingsto the destructured values fromuseKoenigSelectedCardContextis appropriate for managing the visibility settings state in this component.
25-40: Well-implemented toggle function with clear responsibilities.The renaming from what appears to have been
toggleEditModetotoggleVisibilitySettingsbetter reflects this function's purpose. The implementation correctly manages both the visibility settings state and the edit mode dispatch commands.
33-33: Appropriate visibility settings activation.Setting
showVisibilitySettingsto true when entering edit mode aligns with the expected behavior when a user wants to modify visibility settings.
36-36: Properly resetting visibility settings.Resetting
showVisibilitySettingsto false when exiting edit mode ensures the visibility panel is hidden appropriately.
40-40: Correct dependency array update.The addition of
setShowVisibilitySettingsto the dependency array is necessary to ensure the callback is recreated if this function reference changes.
180-180: Consistent prop update with function rename.The update to use
toggleVisibilitySettingsfor theonIndicatorClickprop maintains consistency with the function's new name and purpose.
| // await card.getByTestId('edit-html').click(); | ||
| await card.getByTestId('show-visibility').click(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented-out edit button interaction is acceptable if no longer needed.
Ensure no references to the old edit-html workflow remain if the show-visibility button fully replaces its functionality for changing visibility settings.
🏁 Script executed:
#!/bin/bash
# Confirm there are no stale references to 'edit-html' across the test suite
rg 'edit-html' -A 2Length of output: 1641
Action Required: Remove Legacy 'edit-html' Workflow References
The commented-out edit button interaction in the snippet is acceptable if the legacy edit workflow is deprecated. However, the regex search revealed that active references to the edit-html identifier still exist in both tests and production code:
- In
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx, the element is rendered withdataTestId="edit-html". - In
packages/koenig-lexical/test/e2e/content-visibility.test.js, several test steps verify the presence and interaction foredit-html.
Please confirm that the new show-visibility functionality fully replaces the old edit workflow. If so, update or remove these remaining references to ensure consistency in the codebase.
- since the html card now loads without the settings / visibility panel pre loaded, we don't expect the element to be in the test
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1491 +/- ##
==========================================
+ Coverage 90.92% 90.95% +0.03%
==========================================
Files 187 189 +2
Lines 17589 17683 +94
Branches 1974 1998 +24
==========================================
+ Hits 15992 16083 +91
- Misses 1588 1590 +2
- Partials 9 10 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (1)
14-15: Simplify the boolean toggle.Instead of the ternary expression, consider using the logical NOT operator for a cleaner approach:
- setShowVisibilitySettings(showVisibilitySettings ? false : true); + setShowVisibilitySettings(!showVisibilitySettings);🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(4 hunks)packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js(1 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(5 hunks)packages/koenig-lexical/test/e2e/content-visibility.test.js(4 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/koenig-lexical/test/e2e/content-visibility.test.js (1)
packages/koenig-lexical/test/utils/e2e.js (1)
initialize(10-48)
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (2)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (3)
editor(18-18)isSelected(25-25)React(20-20)packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (3)
editor(14-14)isSelected(23-23)React(13-13)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (2)
packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx (4)
useKoenigSelectedCardContext(35-35)useKoenigSelectedCardContext(35-35)selectedCardKey(6-6)showVisibilitySettings(9-9)packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (1)
useVisibilitySettingsToggle(5-24)
🪛 Biome (1.9.4)
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js
[error] 15-15: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (18)
packages/koenig-lexical/test/e2e/content-visibility.test.js (5)
38-41: Confirm desired default behavior.The test ensures the settings panel remains hidden when the edit button is clicked. Verify that this is the intended UX, as previously opening settings automatically could be a feature. If this is correct, then no changes needed.
Do you want me to run or generate any script-based checks on user workflow consistency?
53-55: Visibility tab usage looks good.Using
show-visibilityandtab-visibilityis consistent with your updated approach. No issues found.
69-70: Remove or update legacy references to “edit-html”.There's a commented-out call to
edit-html. A previous review mentioned removing all references to the old "edit-html" workflow if they're no longer needed. Consider removing these lines if they are truly obsolete.#!/bin/bash # Search for 'edit-html' usage that may still exist in the codebase rg 'edit-html' -A 2
93-94: Consistent with standard toggling pattern.Clicking
show-visibilityand switching tabs ensures you remain in edit mode. This is aligned with the new design. No issues identified.
112-145: Tests thoroughly cover new visibility indicator interactions.These additions provide comprehensive coverage for toggling visibility settings and confirm that edit mode remains unaffected. Great job ensuring test clarity and covering multiple user flows.
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx (6)
6-6: Imports look correct.These plugin commands are used appropriately for selecting and editing cards. No concerns here.
10-10: Hook import is properly introduced.
useVisibilitySettingsToggleis now available, aligning well with the new visibility settings logic.
21-21: Successfully retrieving visibility states.Destructuring
showVisibilitySettingsand its setter from context is a clean approach, making the component’s intent clear.
26-32: Hook usage is consistent.Attaching the
useVisibilitySettingsTogglehook to manage the card’s visibility settings is straightforward and maintains separation of concerns. Good practice for reusability.
80-80: Trailing comma is acceptable.Retaining a trailing comma helps minimize diffs if more parameters are added. No issues found.
172-172: Indicator click callback redefined.Replacing
onIndicatorClickwithtoggleVisibilitySettingscorrectly directs the user flow towards controlling visibility instead of toggling edit mode.packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (7)
12-12: New context hook import looks good.This addition allows the component to properly consume the Koenig selected card context.
14-14: Importing the custom visibility toggle hook is appropriate.Ensures clean separation of toggling logic into a reusable hook.
23-25: Context state destructuring is well implemented.Using
{selectedCardKey, showVisibilitySettings, setShowVisibilitySettings}makes it straightforward to manage and track the card’s visibility settings in a unified manner.
27-32: useEffect cleanup logic is clear and effective.Resetting
showVisibilitySettingstofalsewhenever the card is no longer selected prevents the visibility panel from persisting when the focus shifts away.
69-76: Visibility toggle hook usage is correctly integrated.The component properly updates the editor state and toggles the visibility panel, ensuring consistent behavior with card selection.
106-113: New toolbar item for manual visibility control is well-structured.Providing an explicit onClick handler and icon improves user discoverability and aligns with the planned UX for toggling visibility settings.
126-126: Conditional rendering of SettingsPanel aligns with feature flags and selection state.Combining
isContentVisibilityEnabled,showVisibilitySettings, andcardContext.isSelectedensures the panel only appears under the intended circumstances.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (1)
6-31: Simplify the boolean toggle expression.The hook effectively manages visibility settings for different card types, but the boolean toggle can be simplified.
- setShowVisibilitySettings(showVisibilitySettings ? false : true); + setShowVisibilitySettings(!showVisibilitySettings);🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/koenig-lexical/src/components/KoenigCardWrapper.jsx(5 hunks)packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js(1 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(5 hunks)packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/KoenigCardWrapper.jsx
🧰 Additional context used
🧬 Code Definitions (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (4)
packages/koenig-lexical/src/context/KoenigSelectedCardContext.jsx (4)
useKoenigSelectedCardContext(35-35)useKoenigSelectedCardContext(35-35)selectedCardKey(6-6)showVisibilitySettings(9-9)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (2)
React(96-96)SettingsPanel(17-47)packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (1)
useVisibilitySettingsToggle(6-31)packages/koenig-lexical/src/components/ui/ToolbarMenu.jsx (1)
ToolbarMenuItem(54-76)
🪛 Biome (1.9.4)
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js
[error] 18-18: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (7)
packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (1)
568-582: Well-structured test for the visibility toggle feature.This test effectively validates the behavior described in the PR objectives, showing that the visibility panel can now be triggered through the visibility indicator in the toolbar rather than automatically opening upon card insertion.
packages/koenig-lexical/src/hooks/useVisibilitySettingsToggle.js (1)
1-5: LGTM - Clean import structure.The imports are appropriate for the hook's functionality.
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (5)
12-14: LGTM - Appropriate imports for the new functionality.These imports provide access to the required context and hook for managing visibility settings.
23-32: LGTM - Effective state management for visibility settings.The effect hook properly resets the visibility settings when the card loses selection, ensuring a clean UI experience.
69-76: LGTM - Well-structured hook implementation.The hook is properly configured with all necessary parameters.
107-114: LGTM - Well-implemented visibility toggle button.The toolbar menu item now provides a manual way to toggle visibility settings as described in the PR objectives.
127-141: LGTM - Enhanced conditional rendering of the settings panel.The settings panel is now only rendered when visibility settings are toggled on, and the event propagation is properly handled.
| // If the card is an html card, we toggle the visibility settings differently | ||
| // because we want to show the visibility settings panel while in selected mode | ||
| // instead of entering edit mode | ||
| if ($isHtmlNode(cardNode)) { | ||
| setShowVisibilitySettings(showVisibilitySettings ? false : true); | ||
| if (!isSelected) { | ||
| editor.dispatchCommand(SELECT_CARD_COMMAND, {cardKey: nodeKey, focusEditor: true}); | ||
| } |
There was a problem hiding this comment.
This feels like a code smell that's indicative of a pattern not being quite right. A "generic" hook should not typically have any specific knowledge of what is calling it or what it's acting upon.
If we stick with a hook approach then it would be better if cardNode exposed something that allows the hook to enter this conditional path, similar to the code below where it switches between edit/select based on cardNode exposing whether it has an edit mode.
| const cardNode = $getNodeByKey(nodeKey); | ||
| const clickedDifferentEditor = !cardNode; | ||
| const clickedToolbar = event.target.closest('[data-kg-allow-clickthrough="false"]'); | ||
| const clickedSettingsPanel = event.target.closest('[data-testid="settings-panel"]'); |
There was a problem hiding this comment.
Ideally we would not use testid's in any non-test code, there's no guarantee they'll always be present - e.g. the test id's in our Admin app get stripped out in production builds to save some bytes in the build output - and it creates some potentially invisible coupling as there's usually no expectation that test-specific code would be in use elsewhere.
Better to add our own attributes for selecting anything in app code, similar to the existing data-kg-slash-menu, data-kg-card-menu-item, data-kg-allow-clickthrough etc
There was a problem hiding this comment.
Merging as-is because it's all working well 🙌
However, I do wonder if a different approach where we add something like a new EDIT_CARD_VISIBILITY_COMMAND that the button click events could dispatch 🤔 That would potentially make things more re-usable as it keeps logic centralised and lets Lexical code also dispatch the same command in case we wanted to attach it to keyboard shortcuts etc. The command handler should read some data from the cardNode to determine which type of behaviour to follow (e.g. select-only settings panel vs edit-mode settings panel). I think that approach would also give us a path towards the "open visibility tab when clicking visibility icon" improvement.
- turned off `contentVisibility` flag being on by default in demo app so we can effectively test both off and on states - added test for visibility icon not showing without flag - added test for working visibility icon with flag
4 participants
ref https://linear.app/ghost/issue/PLG-307/change-trigger-for-visibility-panel-on-html-card
We're changing the way the visibility panel is triggered on the html card; instead of opening upon inserting the card, it can now be triggered via an icon in the toolbar.
Added visibility toggle to HTML card toolbar in
HtmlNodeComponent.jsx:ToolbarMenuItemwithdataTestId="show-visibility"andicon="visibility"to trigger visibility settings.toggleVisibilitySettingsfunction for controlled panel display.Implemented new visibility settings panel toggle behaviour in
HtmlNodeComponent.jsx:SettingsPanelto depend onshowVisibilitySettingsstate rather than justisEditingorisSelected.React.useEffectto resetsetShowVisibilitySettings(false)whenisSelectedbecomes false, ensuring panel closure on deselection.Updated
KoenigCardWrapper.jsxto handle visibility settings without entering edit mode:toggleEditModewithtoggleVisibilitySettingsfor theonIndicatorClickprop onCardWrapper.CLICK_COMMANDlistener to avoid triggeringEDIT_CARD_COMMANDwhen clicking settings-related elements (e.g.,clickedSettingsPanelcheck).DESELECT_CARD_COMMANDusage, relying onSELECT_CARD_COMMANDfor selection without edit mode.Created
useVisibilitySettingsToggle.jshook for managing visibility panel state:React.useCallbackto toggleshowVisibilitySettingsstate viasetShowVisibilitySettings.SELECT_CARD_COMMANDwhen the card isn’t selected, ensuring focus without edit mode transition.Modified
HtmlNodeComponent.jsxto show visibility settings only when explicitly toggled:SettingsPanelrendering condition toisContentVisibilityEnabled && showVisibilitySettings && cardContext.isSelectedand removed thecardContext.isEditingcondition.onMouseDownevent handler to prevent event propagation and maintain selection state.Added context state for visibility settings in
KoenigSelectedCardContext.jsx:showVisibilitySettingsandsetShowVisibilitySettingsusingReact.useState(false).Updated E2E tests in
html-card.test.jsandcontent-visibility.test.jsto verify visibility toggle behavior:html-card.test.jsto confirm visibility panel isn’t shown by default in edit mode (expect.not.toBeVisiblefortab-contents-visibility).content-visibility.test.js.data-kg-card-editing="false"attribute persists when toggling visibility settings.Ensured visibility settings panel resets when card is deselected in
HtmlNodeComponent.jsx:React.useEffectto monitorisSelectedand callsetShowVisibilitySettings(false)on deselection.Added visibility indicator toggle functionality in
KoenigCardWrapper.jsxandcontent-visibility.test.js:toggleVisibilitySettingsto the visibility indicator’sonIndicatorClickevent.