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.

Sign up for GitHub

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

Jump to bottom

[@mantine/core] always try centering arrow on target for popover #3065

Merged
merged 2 commits into from Nov 28, 2022
Merged

[@mantine/core] always try centering arrow on target for popover #3065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a80de1f
[@mantine/core] always try centering arrow on target for popover
Nov 26, 2022
bc6d8fc
[@mantine/core] always try centering arrow on target for tooltip
Nov 26, 2022
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
21 changes: 1 addition & 20 deletions src/mantine-core/src/Floating/FloatingArrow/FloatingArrow.tsx
View file
Open in desktop
@@ -1,36 +1,19 @@
import React, { forwardRef } from 'react';
import { useMantineTheme } from '@mantine/styles';
import { getArrowPositionStyles } from './get-arrow-position-styles';
import { FloatingPosition } from '../types';

interface FloatingArrowProps extends React.ComponentPropsWithoutRef<'div'> {
withBorder: boolean;
position: FloatingPosition;
arrowSize: number;
arrowOffset: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need the arrow offset anymore in this component, as the offset is now used as a configuration for the arrow middleware of floating UI directly.

arrowRadius: number;
arrowX: number;
arrowY: number;
visible: boolean;
}

export const FloatingArrow = forwardRef<HTMLDivElement, FloatingArrowProps>(
(
{
withBorder,
position,
arrowSize,
arrowOffset,
arrowRadius,
visible,
arrowX,
arrowY,
...others
},
ref
) => {
const theme = useMantineTheme();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need the theme at all anymore since the RTL direction is already handled by Floating UI


({ withBorder, position, arrowSize, arrowRadius, visible, arrowX, arrowY, ...others }, ref) => {
if (!visible) {
return null;
}
Expand All @@ -43,9 +26,7 @@ export const FloatingArrow = forwardRef<HTMLDivElement, FloatingArrowProps>(
withBorder,
position,
arrowSize,
arrowOffset,
arrowRadius,
dir: theme.dir,
arrowX,
arrowY,
})}
Expand Down
59 changes: 7 additions & 52 deletions src/mantine-core/src/Floating/FloatingArrow/get-arrow-position-styles.ts
View file
Open in desktop
@@ -1,47 +1,6 @@
import { CSSObject } from '@mantine/styles';
import type { FloatingPosition, FloatingSide, FloatingPlacement } from '../types';

function horizontalSide(
placement: FloatingPlacement | 'center',
arrowY: number,
arrowOffset: number
) {
if (placement === 'center') {
return { top: arrowY };
}

if (placement === 'end') {
return { bottom: arrowOffset };
}

if (placement === 'start') {
return { top: arrowOffset };
}

return {};
}

function verticalSide(
placement: FloatingPlacement | 'center',
arrowX: number,
arrowOffset: number,
dir: 'rtl' | 'ltr'
) {
if (placement === 'center') {
return { [dir === 'ltr' ? 'left' : 'right']: arrowX };
}

if (placement === 'end') {
return { [dir === 'ltr' ? 'right' : 'left']: arrowOffset };
}

if (placement === 'start') {
return { [dir === 'ltr' ? 'left' : 'right']: arrowOffset };
}

return {};
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those two functions are not needed anymore, as position is correctly handled by Floating UI, including RTL. So we can directly set left for vertical and top for horizontal sides


const radiusByFloatingSide: Record<
FloatingSide,
keyof Pick<
Expand All @@ -62,22 +21,18 @@ export function getArrowPositionStyles({
position,
withBorder,
arrowSize,
arrowOffset,
arrowRadius,
arrowX,
arrowY,
dir,
}: {
position: FloatingPosition;
withBorder: boolean;
arrowSize: number;
arrowOffset: number;
arrowRadius: number;
arrowX: number;
arrowY: number;
dir: 'rtl' | 'ltr';
}) {
const [side, placement = 'center'] = position.split('-') as [FloatingSide, FloatingPlacement];
const [side] = position.split('-') as [FloatingSide, FloatingPlacement];
const baseStyles = {
width: arrowSize,
height: arrowSize,
Expand All @@ -91,7 +46,7 @@ export function getArrowPositionStyles({
if (side === 'left') {
return {
...baseStyles,
...horizontalSide(placement, arrowY, arrowOffset),
top: arrowY,
right: arrowPosition,
borderLeft: 0,
borderBottom: 0,
Expand All @@ -101,7 +56,7 @@ export function getArrowPositionStyles({
if (side === 'right') {
return {
...baseStyles,
...horizontalSide(placement, arrowY, arrowOffset),
top: arrowY,
left: arrowPosition,
borderRight: 0,
borderTop: 0,
Expand All @@ -111,20 +66,20 @@ export function getArrowPositionStyles({
if (side === 'top') {
return {
...baseStyles,
...verticalSide(placement, arrowX, arrowOffset, dir),
left: arrowX,
bottom: arrowPosition,
borderTop: 0,
[dir === 'ltr' ? 'borderLeft' : 'borderRight']: 0,
borderLeft: 0,
};
}

if (side === 'bottom') {
return {
...baseStyles,
...verticalSide(placement, arrowX, arrowOffset, dir),
left: arrowX,
top: arrowPosition,
borderBottom: 0,
[dir === 'ltr' ? 'borderRight' : 'borderLeft']: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check was actually messing things up for RTL, simply removing it fixes things

borderRight: 0,
};
}

Expand Down
1 change: 0 additions & 1 deletion src/mantine-core/src/Popover/Popover.context.ts
View file
Open in desktop
Expand Up @@ -21,7 +21,6 @@ interface PopoverContext {
width?: PopoverWidth;
withArrow: boolean;
arrowSize: number;
arrowOffset: number;
arrowRadius: number;
trapFocus: boolean;
placement: FloatingPosition;
Expand Down
2 changes: 1 addition & 1 deletion src/mantine-core/src/Popover/Popover.tsx
View file
Open in desktop
Expand Up @@ -201,6 +201,7 @@ export function Popover(props: PopoverProps) {
position: getFloatingPosition(theme.dir, position),
offset: offset + (withArrow ? arrowSize / 2 : 0),
arrowRef,
arrowOffset,
onPositionChange,
positionDependencies,
opened,
Expand Down Expand Up @@ -257,7 +258,6 @@ export function Popover(props: PopoverProps) {
width,
withArrow,
arrowSize,
arrowOffset,
arrowRadius,
placement: popover.floating.placement,
trapFocus,
Expand Down
2 changes: 1 addition & 1 deletion src/mantine-core/src/Popover/PopoverDropdown/PopoverDropdown.tsx
View file
Open in desktop
Expand Up @@ -63,6 +63,7 @@ export function PopoverDropdown({
<Box
{...accessibleProps}
tabIndex={-1}
key={ctx.placement}
ref={ctx.floating}
style={{
...style,
Expand Down Expand Up @@ -92,7 +93,6 @@ export function PopoverDropdown({
position={ctx.placement}
arrowSize={ctx.arrowSize}
arrowRadius={ctx.arrowRadius}
arrowOffset={ctx.arrowOffset}
className={classes.arrow}
/>
</Box>
Expand Down
3 changes: 2 additions & 1 deletion src/mantine-core/src/Popover/use-popover.ts
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ interface UsePopoverOptions {
width: PopoverWidth;
middlewares: PopoverMiddlewares;
arrowRef: React.RefObject<HTMLDivElement>;
arrowOffset: number;
}

function getPopoverMiddlewares(options: UsePopoverOptions) {
Expand All @@ -43,7 +44,7 @@ function getPopoverMiddlewares(options: UsePopoverOptions) {
middlewares.push(inline());
}

middlewares.push(arrow({ element: options.arrowRef }));
middlewares.push(arrow({ element: options.arrowRef, padding: options.arrowOffset }));

return middlewares;
}
Expand Down