Skip to content

fix(PopOver): support translated content and ensure proper outside click behaviour - #232

Open
KatalKavya96 wants to merge 2 commits into
Newton-School:masterfrom
KatalKavya96:fix/popover-outside-click-translated
Open

fix(PopOver): support translated content and ensure proper outside click behaviour#232
KatalKavya96 wants to merge 2 commits into
Newton-School:masterfrom
KatalKavya96:fix/popover-outside-click-translated

Conversation

@KatalKavya96

Copy link
Copy Markdown
Contributor

🔧 Changes Made

  • Added popOverTranslation prop to <PopOver /> for supporting content translation via transform.
  • Ensured PopOver closes on outside click even when content is visually shifted.
  • Applied transform: translate(...) style dynamically based on the passed prop.
  • Updated types in types.ts to support new translation prop.
  • Enhanced test coverage in PopOver.test.tsx to include visual translation behavior.
  • Added fallback safety in .storybook/manager.js to prevent theme crash (tokens undefined).
  • Cleaned up MDX import path in Accordion story to avoid Storybook build failure.

🧪 How to Test

  1. Open Storybook.
  2. Use the PopOver story with popOverTranslation={{ x: 300, y: 200 }}.
  3. Click outside the original position of the PopOver – it should close as expected.
  4. Try different x/y translation values to verify consistent behavior.

✅ Fixes

Closes #100Popover should close on outside click even when visually translated

Test Video

Screen.Recording.2025-05-21.at.7.38.24.AM.mov

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the PopOver component to support optional visual translation and ensures it closes correctly when clicking outside, even if shifted. It also tightens Storybook configuration and corrects a story import path.

  • Introduces popOverTranslation prop and updates types.
  • Adjusts PopOver rendering logic with transform offsets and adds outside-click handling.
  • Updates tests, story MDX import, and Storybook manager fallback.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
ui/elements/PopOver/types.ts Added popOverTranslation prop and updated StyledPopOverContainerProps
ui/elements/PopOver/PopOver.tsx Applies dynamic translation, adds outside-click effect
ui/elements/PopOver/PopOver.test.tsx Adds a basic rendering test for popOverTranslation
ui/elements/PopOver/PopOver.styles.ts Removed extraneous whitespace
stories/elements/Accordion/index.mdx Corrected MDX import path
.storybook/manager.js Provided fallback when themeKey is missing or invalid
Comments suppressed due to low confidence (1)

ui/elements/PopOver/PopOver.test.tsx:68

  • The new test verifies rendering but does not assert that the translation is actually applied. Consider checking the PopOver container’s style.transform or position to ensure the translation offsets are applied as expected.
expect(screen.getByText('PopOver Content')).toBeInTheDocument();

Comment thread ui/elements/PopOver/PopOver.tsx Outdated
setFirstOffsetSet(true);

// Simulate a transform by modifying top and left
setAdjustedOffset({

Copilot AI May 22, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect that sets the initial offset never marks firstOffsetSet as true, so it will re-run continuously. After updating adjustedOffset, you should call setFirstOffsetSet(true) to prevent repeated recalculation.

Copilot uses AI. Check for mistakes.
{...motionProps}
variants={getMotionVariants(direction)}
style={{
transform: `translate(${popOverTranslation?.x || 0}px, ${popOverTranslation?.y || 0}px)`

Copilot AI May 22, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re both adjusting top/left by the translation and applying a CSS transform of the same values, causing double translation. Choose one approach: apply the translation via CSS transform only or adjust the offset calculation, but not both.

Copilot uses AI. Check for mistakes.
Comment thread stories/elements/Accordion/index.mdx Outdated
@@ -1,5 +1,5 @@
import { Meta, DocsPage } from '@storybook/blocks';
import * as AccordionStories from './index.stories';
import * as AccordionStories from "../PopOver/index.stories";

Copilot AI May 22, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MDX for the Accordion story imports from the PopOver folder. It should import Accordion stories (e.g., from './index.stories' or the correct Accordion path) to avoid documentation errors.

Suggested change
import * as AccordionStories from "../PopOver/index.stories";
import * as AccordionStories from "./index.stories";

Copilot uses AI. Check for mistakes.
* @default true
*/
shouldFocusOnFirstElement?: boolean;

Copilot AI May 22, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The new popOverTranslation prop lacks a JSDoc comment. Adding a brief description and default behavior will improve clarity for consumers and maintainers.

Suggested change
/**
* Translation offsets for the PopOver position.
* Allows fine-tuning of the PopOver's placement by specifying x and y offsets.
* @default { x: 0, y: 0 }
*/

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Popover should close on outside click even if the content inside it is translated to different position

2 participants