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: refine arrowStyles CSS to avoid overcast issues with drop shadows #1814

Merged
merged 3 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dropdowns.legacy/src/styled/menu/StyledMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const StyledMenu = styled.ul.attrs<IStyledMenuProps>(props => ({
props.hasArrow &&
arrowStyles(getArrowPosition(props.placement), {
size: `${props.theme.space.base * 2}px`,
inset: '2px',
inset: '1.5px', // More consistent cross-browser positioning with 1.5px
animationModifier: props.isAnimated ? '.is-animated' : undefined
})};

Expand Down
2 changes: 1 addition & 1 deletion packages/dropdowns/src/views/menu/StyledMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export const StyledMenu = styled(StyledListbox).attrs({
props.arrowPosition &&
arrowStyles(props.arrowPosition, {
size: `${props.theme.space.base * 2}px`,
inset: '2px',
inset: '1.5px', // More consistent cross-browser positioning with 1.5px
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Safari handles positioning differently than other browsers. 😠

animationModifier: '[data-garden-animate-arrow="true"]'
})};

Expand Down
12 changes: 9 additions & 3 deletions packages/theming/demo/stories/ArrowStylesStory.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ const StyledDiv = styled.div<Omit<IArgs, 'isAnimated'>>`
box-shadow: ${p =>
p.hasBoxShadow &&
p.theme.shadows.lg(
'8px',
'12px',
getColor({ theme: p.theme, hue: 'chromeHue', shade: 600, transparency: 0.15 })
`${p.theme.space.base * (p.theme.colors.base === 'dark' ? 4 : 5)}px`,
`${p.theme.space.base * (p.theme.colors.base === 'dark' ? 5 : 6)}px`,
getColor({
theme: p.theme,
hue: 'neutralHue',
shade: 1200,
dark: { transparency: p.theme.opacity[800] },
light: { transparency: p.theme.opacity[200] }
})
)};
background-color: ${p => getColor({ theme: p.theme, variable: 'background.primary' })};
padding: ${p => p.theme.space.xxl};
Expand Down
6 changes: 3 additions & 3 deletions packages/theming/src/utils/arrowStyles.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import React from 'react';
import { render } from 'garden-test-utils';
import styled, { ThemeProps, DefaultTheme } from 'styled-components';
import { math } from 'polished';
import arrowStyles, { exponentialSymbols } from './arrowStyles';
import { math, stripUnit } from 'polished';
import arrowStyles from './arrowStyles';
import { ArrowPosition } from '../types';

interface IStyledDivProps extends ThemeProps<DefaultTheme> {
Expand All @@ -29,7 +29,7 @@ const StyledDiv = styled.div<IStyledDivProps>`
`;

const getArrowSize = (size = '6px') => {
return math(`${size} * 2 / sqrt(2)`, exponentialSymbols);
return `${Math.round(((stripUnit(size) as number) * 2) / Math.sqrt(2))}px`;
};

const getArrowInset = (inset: string, size?: string) => {
Expand Down
96 changes: 34 additions & 62 deletions packages/theming/src/utils/arrowStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import { css, keyframes } from 'styled-components';
import { math } from 'polished';
import { math, stripUnit } from 'polished';
import { ArrowPosition } from '../types';

type ArrowOptions = {
Expand All @@ -15,24 +15,6 @@ type ArrowOptions = {
animationModifier?: string;
};

// Workaround for https://github.com/styled-components/polished/issues/550
export const exponentialSymbols = {
symbols: {
sqrt: {
func: {
symbol: 'sqrt',
f: (a: number) => Math.sqrt(a),
notation: 'func',
precedence: 0,
rightToLeft: 0,
argCount: 1
},
symbol: 'sqrt',
regSymbol: 'sqrt\\b'
}
}
};

const animationStyles = (position: ArrowPosition, modifier: string) => {
const property = position.split('-')[0];
/**
Expand All @@ -55,40 +37,35 @@ const animationStyles = (position: ArrowPosition, modifier: string) => {
const positionStyles = (position: ArrowPosition, size: string, inset: string) => {
const margin = math(`${size} / -2`);
const placement = math(`${margin} + ${inset}`);
let clipPath;
let transform;
let positionCss;
let propertyRadius: string;

if (position.startsWith('top')) {
propertyRadius = 'border-bottom-right-radius';
clipPath = 'polygon(100% 0, 100% 1px, 1px 100%, 0 100%, 0 0)';
transform = 'rotate(-135deg)';
positionCss = css`
top: ${placement};
right: ${position === 'top-right' && size};
left: ${position === 'top' ? '50%' : position === 'top-left' && size};
margin-left: ${position === 'top' && margin};
`;
} else if (position.startsWith('right')) {
propertyRadius = 'border-bottom-left-radius';
clipPath = 'polygon(100% 0, 100% 100%, calc(100% - 1px) 100%, 0 1px, 0 0)';
transform = 'rotate(-45deg)';
positionCss = css`
top: ${position === 'right' ? '50%' : position === 'right-top' && size};
right: ${placement};
bottom: ${position === 'right-bottom' && size};
margin-top: ${position === 'right' && margin};
`;
} else if (position.startsWith('bottom')) {
propertyRadius = 'border-top-left-radius';
clipPath = 'polygon(100% 0, calc(100% - 1px) 0, 0 calc(100% - 1px), 0 100%, 100% 100%)';
transform = 'rotate(45deg)';
positionCss = css`
right: ${position === 'bottom-right' && size};
bottom: ${placement};
left: ${position === 'bottom' ? '50%' : position === 'bottom-left' && size};
margin-left: ${position === 'bottom' && margin};
`;
} else if (position.startsWith('left')) {
propertyRadius = 'border-top-right-radius';
clipPath = 'polygon(0 100%, 100% 100%, 100% calc(100% - 1px), 1px 0, 0 0)';
transform = 'rotate(135deg)';
positionCss = css`
top: ${position === 'left' ? '50%' : position === 'left-top' && size};
bottom: ${size};
Expand All @@ -98,21 +75,14 @@ const positionStyles = (position: ArrowPosition, size: string, inset: string) =>
}

/**
* 1. Round-off portion of the foreground square opposite the arrow tip
* (improved layout for IE which doesn't support 'clip-path').
* 2. Clip portion of the foreground square opposite the arrow tip so that it
* doesn't interfere with container content.
* 3. Arrow positioning on the base element.
* 1. Rotate the clipping mask depending on arrow position.
* 2. Arrow positioning on the base element.
*/
return css`
&::before {
${propertyRadius!}: 100%; /* [1] */
clip-path: ${clipPath}; /* [2] */
}

&::before,
&::after {
${positionCss}/* [3] */
transform: ${transform}; /* [1] */
${positionCss}; /* [2] */
}
`;
};
Expand Down Expand Up @@ -150,45 +120,47 @@ const positionStyles = (position: ArrowPosition, size: string, inset: string) =>
* @component
*/
export default function arrowStyles(position: ArrowPosition, options: ArrowOptions = {}) {
const size = options.size || '6px';
const inset = options.inset || '0';
const squareSize = math(`${size} * 2 / sqrt(2)`, exponentialSymbols);
const size = options.size === undefined ? 6 : (stripUnit(options.size) as number);
const squareSize = `${Math.round((size * 2) / Math.sqrt(2))}px`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm glad this works. I was having problems and needed Math.floor.

const afterOffset = 0;
const beforeOffset = afterOffset + 2;

/**
* 1. Set base positioning for an element with an arrow.
* 2. Allow any border inherited by `::after` to show through.
* 3. Border styling and box-shadow will be automatically inherited from the
* parent element.
* 4. Apply shared sizing properties to ::before and ::after.
* 2. Apply shared properties to ::before and ::after.
* 3. Display border with inherited border-color
* 4. Clip the outer square forming the arrow border into a triangle so that the
* border merge with the container's.
* 5. Clip the inner square forming the arrow body into a triangle so that it
* doesn't interfere with container content.
*/
return css`
position: relative; /* [1] */

&::before {
&::before,
&::after {
/* [2] */
position: absolute;
border-width: inherit;
border-style: inherit;
border-color: transparent;
background-clip: content-box;
width: ${squareSize};
height: ${squareSize};
content: '';
box-sizing: inherit;
}

&::after {
/* [3] */
z-index: -1;
border: inherit;
box-shadow: inherit;
jzempel marked this conversation as resolved.
Show resolved Hide resolved
&::before {
border-color: inherit; /* [3] */
background-color: transparent;
clip-path: polygon(100% ${beforeOffset}px, ${beforeOffset}px 100%, 100% 100%); /* [4] */
}

&::before,
&::after {
/* [4] */
position: absolute;
transform: rotate(45deg);
border-color: transparent;
background-clip: content-box;
background-color: inherit;
box-sizing: inherit;
width: ${squareSize};
height: ${squareSize};
content: '';
clip-path: polygon(100% ${afterOffset}px, ${afterOffset}px 100%, 100% 100%); /* [5] */
}

${positionStyles(position, squareSize, inset)};
Expand Down