Skip to content

Allow an arrow to be added to popover component - #2011

Merged
acelaya merged 1 commit into
mainfrom
popover-arrow
Jun 18, 2025
Merged

Allow an arrow to be added to popover component#2011
acelaya merged 1 commit into
mainfrom
popover-arrow

Conversation

@acelaya

@acelaya acelaya commented Jun 17, 2025

Copy link
Copy Markdown
Contributor

Add new arrowed?: boolean prop to Popover that allows an arrow/triangle pointing to the anchor element to be rendered.

The arrow is aligned to the same side as the popover, and points up or down depending on the Popover placement.

popover-arrow-2025-06-17_15.38.44.mp4

The functionality is very simple at this point and is not very customizable, but let's wait to see what new requirements appear before making it more complex.

Our initial use case is using the Popover for the share annotation behavior in the sidebar and the moderation queue. This is how it looks with the code from this PR:

popover-arrow-sidebar-2025-06-17_15.43.14.mp4

Tip

This PR is easier to review ignoring whitespaces

TODO

  • Document new prop
  • Add tests

@acelaya
acelaya force-pushed the popover-arrow branch 2 times, most recently from fb669c5 to d2f0429 Compare June 17, 2025 13:53
@codecov

codecov Bot commented Jun 17, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3fd5193) to head (edaa250).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2011   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        70           
  Lines         1319      1329   +10     
  Branches       489       497    +8     
=========================================
+ Hits          1319      1329   +10     

☔ 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.

@acelaya
acelaya force-pushed the popover-arrow branch 3 times, most recently from 636447c to 9e542b0 Compare June 18, 2025 07:58
@acelaya
acelaya requested a review from robertknight June 18, 2025 07:58
@acelaya
acelaya marked this pull request as ready for review June 18, 2025 07:58
variant === 'panel' && [
'max-h-80 overflow-y-auto overflow-x-hidden',
'rounded border bg-white shadow hover:shadow-md focus-within:shadow-md',
],

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.

The outermost Popover element is now "unstyled", and there's a new inner element wrapping children which is the one receiving styling classes.

This allows the arrow to be displayed inside the "unstyled" container, and keep the other one looking as before.

There's one consideration. The classes prop, which intuitively one would expect to be added to the outermost element, is now added to the "styled" inner element, to ensure it keeps looking the same. Maybe we should document this in the prop and in future we can add a new containerClasses prop if we need to provide classes to the outermost element.

@robertknight

robertknight commented Jun 18, 2025

Copy link
Copy Markdown
Contributor

Safari is showing a border between the arrow and the popover:

Safari arrow

From some initial experimentation it seems this is connected with the way that the position of the popover set by top is not a whole number of pixels. For me the top position is calculated as calc(Npx + 0.15rem) where N is some integer. Since the root font size is 16px, 0.15rem = 2.4px. Applying this diff resolved the problem:

diff --git a/src/components/feedback/Popover.tsx b/src/components/feedback/Popover.tsx
index d8f6639..b98dae8 100644
--- a/src/components/feedback/Popover.tsx
+++ b/src/components/feedback/Popover.tsx
@@ -15,7 +15,7 @@ import { downcastRef } from '../../util/typing';
 import { PointerDownIcon, PointerUpIcon } from '../icons';
 
 /** Small space to apply between the anchor element and the popover */
-const POPOVER_ANCHOR_EL_GAP = '.15rem';
+const POPOVER_ANCHOR_EL_GAP = '3px';
 
 /**
  * Space in pixels to apply between the popover and the viewport sides to

I would make sure to refer back to this comment in a change, since the issue is rather subtle. Also if all the units are pixels you can do the computation in JS and round there.

@acelaya

acelaya commented Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

Safari is showing a border between the arrow and the popover:

Thanks! Good catch.

@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.

The main feedback I have is to avoid using the word "arrowed". Use the phrase "Popover with arrow" in the example and either arrow or showArrow or withArrow as the prop name. I also found a render issue in Safari that seems to occur when the popover's calculated top position is not a whole number of pixels. Changing the value so that it is a whole number of pixels resolves the issue.

Comment thread src/components/feedback/Popover.tsx Outdated
Comment thread src/components/feedback/Popover.tsx Outdated
Comment thread src/components/feedback/Popover.tsx Outdated
Comment thread src/components/feedback/test/Popover-test.js Outdated
@acelaya
acelaya merged commit 2644da6 into main Jun 18, 2025
@acelaya
acelaya deleted the popover-arrow branch June 18, 2025 12:11
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