Skip to content

Allow to customize initial Popover placement - #2000

Merged
acelaya merged 2 commits into
mainfrom
popover-placement
Jun 17, 2025
Merged

Allow to customize initial Popover placement#2000
acelaya merged 2 commits into
mainfrom
popover-placement

Conversation

@acelaya

@acelaya acelaya commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Add a new placement?: 'above' | 'below' property to the Popover, so that it is possible to customize the default placement of the popover relative to the anchor.

Up until now, popovers would always render below the anchor when there's enough space.

The component can still decide to render in the opposite placement if selected one does not allow for it to render inside the viewport.

popover-placement-2025-06-17_10.01.03.mp4

Test steps

  1. Go to http://localhost:4001/feedback-popover
  2. Scroll down to the placement property doc, which has an example of above and below popovers.
  3. While the buttons are more or less in the middle, popovers should show in the placement indicated by every button.
  4. If the buttons are close to the top or bottom, their placements may change.
  5. Resize the window so that the popovers do not fit neither above or below.
  6. They should now show in the direction where there's more available space, even if they don't fit.

Warning

Ideally, when the popover does not fit neither above or below, it should re-position and overlap the anchor element, to make sure it is kept in the viewport.
That is not part of this PR, and will be addressed as part of #1921

TODO

  • Ensure placement falls back only if the popover does not fit in available space, not if there's more space in the opposite direction.
  • Add tests.
  • Document new prop.

Comment thread src/components/feedback/Popover.tsx
@codecov

codecov Bot commented Jun 13, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7109fdc) to head (e1c48ce).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2000   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        70           
  Lines         1314      1319    +5     
  Branches       484       489    +5     
=========================================
+ Hits          1314      1319    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Comment thread src/components/feedback/Popover.tsx Outdated
Comment thread src/components/feedback/Popover.tsx Outdated
Comment thread src/components/feedback/Popover.tsx
@acelaya
acelaya force-pushed the popover-placement branch 6 times, most recently from 86091b7 to d2bff47 Compare June 17, 2025 08:03
@acelaya
acelaya force-pushed the popover-placement branch from d2bff47 to 2f4c499 Compare June 17, 2025 08:22
Comment thread vitest.config.js
headless: true,
screenshotFailures: false,
instances: [{ browser: 'chromium' }],
viewport: { width: 1024, height: 768 },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tests were a bit flaky without this, due to the default vitest resolution.

This is the viewport we have set in the rest of the projects, but it was overlooked here because it was the first project migrated to vitest.

// grows further than the opposite side of the anchor element
{
children: 'very long text'.repeat(4),
children: 'very long text'.repeat(8),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adjustment needed for the new viewport size used in tests. The viewport is now wider so the text needs to be longer.

@acelaya
acelaya requested a review from robertknight June 17, 2025 08:43
@acelaya
acelaya marked this pull request as ready for review June 17, 2025 08:43

@robertknight robertknight left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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.

2 participants