Koenig icon indicating copy to clipboard operation
Koenig copied to clipboard

Published 20 hours ago • verified publisher icon TryGhost

Added visibility toggle to html card toolbar

Open sanne-san opened this issue 8 months ago • 2 comments

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:

    • Introduced a new ToolbarMenuItem with dataTestId="show-visibility" and icon="visibility" to trigger visibility settings.
    • Wired it to the toggleVisibilitySettings function for controlled panel display.

  • Implemented new visibility settings panel toggle behaviour in HtmlNodeComponent.jsx:

    • Updated conditional rendering of SettingsPanel to depend on showVisibilitySettings state rather than just isEditing or isSelected.
    • Added React.useEffect to reset setShowVisibilitySettings(false) when isSelected becomes false, ensuring panel closure on deselection.

  • Updated KoenigCardWrapper.jsx to handle visibility settings without entering edit mode:

    • Replaced toggleEditMode with toggleVisibilitySettings for the onIndicatorClick prop on CardWrapper.
    • Modified CLICK_COMMAND listener to avoid triggering EDIT_CARD_COMMAND when clicking settings-related elements (e.g., clickedSettingsPanel check).
    • Removed DESELECT_CARD_COMMAND usage, relying on SELECT_CARD_COMMAND for selection without edit mode.

  • Created useVisibilitySettingsToggle.js hook for managing visibility panel state:

    • Added a new hook that uses React.useCallback to toggle showVisibilitySettings state via setShowVisibilitySettings.
    • Dispatches SELECT_CARD_COMMAND when the card isn’t selected, ensuring focus without edit mode transition.

  • Modified HtmlNodeComponent.jsx to show visibility settings only when explicitly toggled:

    • Changed SettingsPanel rendering condition to isContentVisibilityEnabled && showVisibilitySettings && cardContext.isSelected and removed the cardContext.isEditing condition.
    • Added onMouseDown event handler to prevent event propagation and maintain selection state.

  • Added context state for visibility settings in KoenigSelectedCardContext.jsx:

    • Extended context with showVisibilitySettings and setShowVisibilitySettings using React.useState(false).

  • Updated E2E tests in html-card.test.js and content-visibility.test.js to verify visibility toggle behavior:

    • Added test in html-card.test.js to confirm visibility panel isn’t shown by default in edit mode (expect.not.toBeVisible for tab-contents-visibility).
    • Enhanced content-visibility.test.js.
    • Verified data-kg-card-editing="false" attribute persists when toggling visibility settings.

  • Ensured visibility settings panel resets when card is deselected in HtmlNodeComponent.jsx:

    • Used React.useEffect to monitor isSelected and call setShowVisibilitySettings(false) on deselection.

  • Added visibility indicator toggle functionality in KoenigCardWrapper.jsx and content-visibility.test.js:

    • Linked toggleVisibilitySettings to the visibility indicator’s onIndicatorClick event.
    • Added E2E test to confirm clicking the indicator toggles the settings panel without entering edit mode.

sanne-san avatar Mar 25 '25 08:03 sanne-san

Walkthrough

The pull request introduces a new context hook, useKoenigSelectedCardContext, to manage the state related to the selected card within the HtmlNodeComponent. A new state variable, showVisibilitySettings, is added along with its setter, setShowVisibilitySettings. A React.useEffect hook resets showVisibilitySettings to false when the component is no longer selected. The toggleVisibilitySettings function is defined using the useVisibilitySettingsToggle hook and is invoked in the ToolbarMenuItem labeled "Visibility." The rendering condition for the SettingsPanel is modified to depend on both isContentVisibilityEnabled and showVisibilitySettings. Additionally, new test cases are added to verify the behavior of the visibility panel in different editing states.

Possibly related PRs

  • TryGhost/Koenig#1436: The changes in the main PR are related to the retrieved PR that enhances visibility rendering for the Call to Action card, as both involve modifications to visibility handling logic and the introduction of new functions for managing visibility states.
  • TryGhost/Koenig#1424: The changes in the main PR are related to the retrieved PR as both involve modifications to visibility management logic, specifically through the introduction of new structures and functions for handling visibility settings in the HTML node context.
  • TryGhost/Koenig#1440: The changes in the main PR are related to those in the retrieved PR as both involve the management and toggling of visibility settings within components, specifically focusing on the HtmlNodeComponent and CtaCard, respectively.

Suggested reviewers

  • ronaldlangeveld

[!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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

packages/koenig-lexical/test/e2e/cards/html-card.test.js

Oops! 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.jsx

Oops! 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.jsx

Oops! 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.

  • 1 others
📜 Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 342233d9adf6a1aeaaea934c2505a2607e53ea4a and b83268c33e6a35b96bec487681410f3a6c5c6758.

📒 Files selected for processing (4)
  • packages/koenig-lexical/demo/DemoApp.jsx (1 hunks)
  • packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (5 hunks)
  • packages/koenig-lexical/test/e2e/cards/call-to-action-card.test.js (7 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/test/e2e/cards/call-to-action-card.test.js
🧰 Additional context used
🧬 Code Definitions (2)
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)
packages/koenig-lexical/test/e2e/cards/html-card.test.js (1)
packages/koenig-lexical/test/utils/e2e.js (2)
  • focusEditor (58-61)
  • initialize (10-48)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Node 22.13.1
  • GitHub Check: Node 20.11.1
🔇 Additional comments (13)
packages/koenig-lexical/demo/DemoApp.jsx (1)

53-53: Updated default configuration to disable content visibility

The default value for contentVisibility has been changed from true to false, which aligns with the PR objective of changing how the visibility panel is triggered. This ensures that by default, users must explicitly activate the visibility panel through the toolbar icon rather than having it appear automatically.

packages/koenig-lexical/test/e2e/cards/html-card.test.js (4)

178-189: Test confirms absence of visibility icon when feature is disabled

This test effectively verifies that the visibility icon doesn't appear in the toolbar when the content visibility feature is disabled by default. The test follows best practices by:

  1. Creating the HTML card
  2. Checking for the visibility of the toolbar
  3. Asserting that the visibility icon is not present

This aligns well with the PR's objective of making the visibility panel trigger manual.

192-206: Well-structured test setup for content visibility feature

The test suite properly isolates tests for the enabled content visibility feature by:

  1. Creating a separate describe block
  2. Using the appropriate URI parameters to enable the feature (&labs=contentVisibility)
  3. Following test best practices with proper setup and teardown

This organization makes the test intentions clear and keeps the test cases focused.

207-221: Complete test coverage for visibility icon functionality

This test thoroughly validates the visibility icon's behavior when the content visibility feature is enabled:

  1. Verifies that the visibility icon appears in the toolbar
  2. Confirms that clicking the icon opens the visibility settings panel

The test covers the complete interaction flow, providing confidence in the feature's implementation.

223-229: Test ensures visibility panel doesn't show automatically

This test correctly verifies that the visibility settings panel doesn't appear automatically when entering edit mode, which confirms the intended change in behavior. This ensures that users must explicitly click the visibility icon to see these settings.

packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (8)

12-12: Added context hook for selected card state

The import of useKoenigSelectedCardContext is appropriate for accessing the selected card state, which is necessary for managing the visibility settings panel.

14-14: Added visibility settings toggle hook

The import of useVisibilitySettingsToggle properly separates the toggle logic into a reusable hook, promoting code reusability and separation of concerns.

23-26: Correctly accessing selected card context

The destructuring of context values and creation of the isSelected variable provides a clean way to check if the current card is selected, which is essential for managing the visibility settings panel state.

28-32: Added effect to reset visibility settings when unselected

This useEffect hook ensures that the visibility settings panel is closed when the card loses selection, preventing it from remaining open when the user moves to another card. This is a critical improvement for the user experience.

69-76: Implemented visibility settings toggle

The toggleVisibilitySettings function is properly implemented using the hook with all the necessary parameters:

  • editor
  • nodeKey
  • isSelected state
  • visibility settings state
  • setter function
  • editing state

This encapsulates all the logic needed for toggling the visibility settings panel.

107-118: Added conditional toolbar menu item for visibility

The visibility icon is correctly wrapped in a conditional block that only renders it when the content visibility feature is enabled. The menu item has:

  • Proper test ID for easy testing
  • Appropriate icon
  • Active state tied to the visibility settings state
  • Clear label
  • Correctly wired onClick handler

This implements the core UI change required for the PR objective.

131-131: Updated condition for rendering settings panel

The condition now correctly checks both the feature flag and the showVisibilitySettings state before rendering the panel, ensuring it only appears when explicitly triggered by the user.

136-139: Improved event handling for settings panel

The onMouseDown handler now includes e.stopPropagation() to prevent event bubbling, which helps avoid unintended interactions when interacting with the settings panel. This improves the user experience by ensuring events don't propagate unexpectedly.

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

coderabbitai[bot] avatar Mar 25 '25 08:03 coderabbitai[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.95%. Comparing base (6ccedee) to head (9b18548). Report is 19 commits behind head on main.

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov-commenter avatar Mar 27 '25 04:03 codecov-commenter