fix: refine arrowStyles CSS to avoid overcast issues with drop shadows
#1814
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ze-flo
changed the title
arrowStyle css to avoid issues with drop-shadows
ze-flo
changed the title
arrowStyle css to avoid issues with drop-shadowsarrowStyle css to avoid issues with drop shadows
ze-flo
changed the title
arrowStyle css to avoid issues with drop shadowsarrowStyle CSS to avoid overcast issues with drop shadows
ze-flo
commented
May 23, 2024
| @@ -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 | |||
There was a problem hiding this comment.
Safari handles positioning differently than other browsers. 😠
jzempel
changed the title
arrowStyle CSS to avoid overcast issues with drop shadowsarrowStyles CSS to avoid overcast issues with drop shadows
jzempel
requested changes
May 30, 2024
There was a problem hiding this comment.
This is feeling really good. Can we pick up the story updates from #1812 to get a more precise match with real component usage.
Comment on lines
78
to
79
| * 1. Arrow positioning on the base element. | ||
| * 2. Rotate the clipping mask depending on arrow position. |
There was a problem hiding this comment.
[nit] please re-order so the comments read top-to-bottom with the CSS below
| 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`; |
There was a problem hiding this comment.
I'm glad this works. I was having problems and needed Math.floor.
Comment on lines
133
to
136
| * 4. Clip the inner square forming the arrow body into a triangle so that it | ||
| * doesn't interfere with container content. | ||
| * 5. Clip the outer square forming the arrow border into a triangle so that the | ||
| * border merge with the container's. |
There was a problem hiding this comment.
[nit] ditto re: top-to-bottom order
7 tasks
jzempel
approved these changes
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Improves
arrowStylesto avoid overcast issues on arrow border when drop shadows are applied to the arrow container.Using an extreme case to highlight the impact of drop shadows on the arrow border
Detail
::before(in pink) inherits the border styles from container. The clipping mask is set so that the border smoothly merges with the container's (usinginset == 0). The overlap is meant to prevent a gap due to pixel rounding.::after(in green) has a transparent border and a clipping mask that slightly extends upward to conceal the container's border without hiding its content.Checklist
reviewer)
npm start)⬅️ renders as expected with reversed (RTL) direction🤘 renders as expected with Bedrock CSS (?bedrock)♿ tested for WCAG 2.1 AA accessibility compliance