-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create new Popover component #1791
Conversation
5c46c89
to
97596e2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1791 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 67 68 +1
Lines 1203 1220 +17
Branches 455 465 +10
=========================================
+ Hits 1203 1220 +17 ☔ View full report in Codecov by Sentry. |
b9c0276
to
ce4e691
Compare
popover={listboxAsPopover ? 'auto' : undefined} | ||
data-testid="select-popover" | ||
<Popover | ||
anchorElementRef={wrapperRef} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not related with the extraction of Popover
specifically (this would have been anchorElementRef={buttonRef}
), but I realized the border is set in the container, not the button, which makes popover positioning and sizing calculation fail if it's done relative to the button.
It is easier to see the issue with a thicker border.
2da07d5
to
59a92b2
Compare
{open && | ||
(children ?? ( | ||
<> | ||
Content of popover | ||
<button | ||
data-testid="inner-button" | ||
onClick={() => setOpen(prev => !prev)} | ||
> | ||
Focusable element inside popover | ||
</button> | ||
</> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The popover should not render its children when it's closed, but a bunch of Select
tests break if that behavior is changed.
I will tackle that separately, checking what's the best way to fix the Select tests. Perhaps mocking the Popover
component in that case is the right approach, now that the Popover has its own test.
For now, this condition allows to test it as if it was already behaving like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turnso out fixing those tests was easy https://github.com/hypothesis/frontend-shared/pull/1792/files#r1853917255
59a92b2
to
b355b33
Compare
src/components/feedback/Popover.tsx
Outdated
export type PopoverProps = { | ||
children?: ComponentChildren; | ||
classes?: string | string[]; | ||
variant?: 'panel' | 'custom'; // TODO 'tooltip' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Panel" variant adds the style used by Select
, but it's likely the one we'll use for at-mentions as well.
It has:
- White background
- Box shadow
- Border
- Rounded corners
- Padding
The "custom" variant will simply leave it unstyled, and we could add other variants in future, like a "tooltip" one which sets black transparent background, smaller padding, etc.
48f3633
to
0dcf3ce
Compare
0dcf3ce
to
3021966
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All test cases that have been removed from here are now part of Popover-test.js (with a few variables renamed and titles changed to reference "popover" instead of "listbox").
However, I left a couple of overlapping tests that have helped catch regressions in the past, like the test that checks focus returns to the toggle button when the listbox is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a few minor questions to clarify comments.
3021966
to
8e767d5
Compare
Extract the popover element and the logic to dynamically position it from
Select
and into its ownPopover
module.This will allow us to use this component in other contexts.
Considerations
The popover currently requires an anchor element to be provided, and calculates its position and size based on that.
For now, I decided to keep some decisions made, like the fact that the popover appears below the anchor element unless there's not enough space, in which case it appears above it.
It also matches the anchor element's size, unless it can't fit its content, in which case it grows.
It should be possible to customize both of those (define initial position, determine how to calculate size, etc.), but I'm not going to change that as part of this PR.
We may also need to support a different way to determine the anchor, like providing a rectangle rather than an anchor element.
Out of scope
Popover
in this PR, as it has enough changes already.Separately, I will add a documentation page explaining the component, how it works and behaves by default, and including some examples.
EDIT: Addressed in Add documentation page for Popover component #1793
EDIT: This is addressed in Do not render Popover children while closed #1792
Testing steps
Sanity check that all examples in http://localhost:4001/input-select keep working as expected
TODO
Select
, if they overlap with Popover tests