Skip to content
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

fix(Popover): arrow offset when scrolling offscreen #5287

Merged
merged 2 commits into from Jul 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/DropdownMenu.tsx
Expand Up @@ -4,6 +4,7 @@ import React, { useContext } from 'react';
import {
useDropdownMenu,
UseDropdownMenuValue,
UseDropdownMenuOptions,
} from 'react-overlays/DropdownMenu';
import useMergedRefs from '@restart/hooks/useMergedRefs';
import NavbarContext from './NavbarContext';
Expand All @@ -23,7 +24,7 @@ export interface DropdownMenuProps extends BsPrefixPropsWithChildren {
alignRight?: boolean;
onSelect?: SelectCallback;
rootCloseEvent?: 'click' | 'mousedown';
popperConfig?: { modifiers?: any };
popperConfig?: UseDropdownMenuOptions['popperConfig'];
}

type DropdownMenu = BsPrefixRefForwardingComponent<'div', DropdownMenuProps>;
Expand Down Expand Up @@ -92,7 +93,7 @@ const DropdownMenu: DropdownMenu = React.forwardRef(
renderOnMount,
// Need to define the default "as" during prop destructuring to be compatible with styled-components github.com/react-bootstrap/react-bootstrap/issues/3595
as: Component = 'div',
popperConfig = {},
popperConfig,
...props
}: DropdownMenuProps,
ref,
Expand All @@ -116,7 +117,7 @@ const DropdownMenu: DropdownMenu = React.forwardRef(
usePopper: !isNavbar,
popperConfig: {
...popperConfig,
modifiers: marginModifiers.concat(popperConfig.modifiers || []),
modifiers: marginModifiers.concat(popperConfig?.modifiers || []),
},
}) as UseDropdownMenuValueHack;

Expand Down
11 changes: 5 additions & 6 deletions src/Overlay.tsx
Expand Up @@ -182,19 +182,18 @@ function Overlay({
props: overlayProps,
arrowProps,
show,
// @ts-ignore
state,
scheduleUpdate,
update,
placement,
outOfBoundaries,
state,
...props
}) => {
wrapRefs(overlayProps, arrowProps);
const popper = Object.assign(popperRef.current, {
state,
scheduleUpdate,
scheduleUpdate: update,
placement,
outOfBoundaries,
outOfBoundaries:
state?.modifiersData.hide?.isReferenceHidden || false,
});

if (typeof overlay === 'function')
Expand Down
14 changes: 9 additions & 5 deletions src/OverlayTrigger.tsx
Expand Up @@ -5,6 +5,7 @@ import useTimeout from '@restart/hooks/useTimeout';
import safeFindDOMNode from 'react-overlays/safeFindDOMNode';
import warning from 'warning';
import { useUncontrolledProp } from 'uncontrollable';
import { Modifier } from 'react-overlays/esm/usePopper';
import Overlay, { OverlayChildren, OverlayProps } from './Overlay';

export type OverlayTriggerType = 'hover' | 'click' | 'focus';
Expand Down Expand Up @@ -264,13 +265,14 @@ function OverlayTrigger({

// We add aria-describedby in the case where the overlay is a role="tooltip"
// for other cases describedby isn't appropriate (e.g. a popover with inputs) so we don't add it.
const ariaModifier = {
const ariaModifier: Modifier<'ariaDescribedBy', {}> = {
name: 'ariaDescribedBy',
enabled: true,
phase: 'afterWrite',
effect: ({ state }) => {
return () => {
state.elements.reference.removeAttribute('aria-describedby');
if ('removeAttribute' in state.elements.reference)
state.elements.reference.removeAttribute('aria-describedby');
};
},
fn: ({ state }) => {
Expand All @@ -279,7 +281,11 @@ function OverlayTrigger({
if (!show || !reference) return;

const role = popper.getAttribute('role') || '';
if (popper.id && role.toLowerCase() === 'tooltip') {
if (
popper.id &&
role.toLowerCase() === 'tooltip' &&
'setAttribute' in reference
) {
reference.setAttribute('aria-describedby', popper.id);
}
},
Expand Down Expand Up @@ -318,8 +324,6 @@ function OverlayTrigger({
{...props}
popperConfig={{
...popperConfig,
// TODO: fix typing
// @ts-ignore
modifiers,
}}
show={show}
Expand Down
7 changes: 1 addition & 6 deletions src/Popover.tsx
Expand Up @@ -109,12 +109,7 @@ const Popover: Popover = (React.forwardRef<HTMLDivElement, PopoverProps>(
)}
{...props}
>
<div
className="arrow"
{...arrowProps}
// this prevents an error if you render a Popover without arrow props, like in a test
style={arrowProps ? { ...arrowProps.style, margin: 0 } : undefined}
/>
<div className="arrow" {...arrowProps} />
{content ? <PopoverContent>{children}</PopoverContent> : children}
</div>
);
Expand Down
39 changes: 37 additions & 2 deletions src/usePopperMarginModifiers.tsx
Expand Up @@ -37,7 +37,7 @@ export default function usePopperMarginModifiers(): [
overlayRef.current = overlay;
}, []);

const modifier = useMemo(() => {
const offset = useMemo(() => {
return {
name: 'offset',
options: {
Expand All @@ -61,5 +61,40 @@ export default function usePopperMarginModifiers(): [
},
};
}, [margins]);
return [callback, [modifier]];

// Converts popover arrow margin to arrow modifier padding
const popoverArrowMargins = useMemo(() => {
return {
name: 'popoverArrowMargins',
enabled: true,
phase: 'main',
requiresIfExists: ['arrow'],
effect({ state }) {
if (
!overlayRef.current ||
!state.elements.arrow ||
!hasClass(overlayRef.current, 'popover') ||
!state.modifiersData['arrow#persistent']
Copy link
Member

Choose a reason for hiding this comment

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

question: where does #persistent come from? is this really an id?

Copy link
Member Author

Choose a reason for hiding this comment

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

its a special key Popper adds, nothing unique about it tho aside from Popper picked it

) {
return undefined;
}

const { top, right } = getMargins(state.elements.arrow);
const padding = top || right;
state.modifiersData['arrow#persistent'].padding = {
top: padding,
left: padding,
right: padding,
bottom: padding,
};
state.elements.arrow.style.margin = '0';

return () => {
if (state.elements.arrow) state.elements.arrow.style.margin = '';
};
},
};
}, []);

return [callback, [offset, popoverArrowMargins]];
}