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 #1812

Closed
wants to merge 3 commits into from
Closed

fix: refine arrowStyles CSS #1812

wants to merge 3 commits into from

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented May 22, 2024

Description

Dark mode is highlighting a shadow overcast problem with Garden arrows. This PR update swaps the ::after border "peek-through" with the following:

  • border is inherited on the front side ::before pseudo (box-shadow continues to be inherited on the back side ::after)
  • border inheritance is set to none for the non-arrow sides of the element
  • the border radius hack for IE11 is removed – clip-path does the job for all supported browsers

The result is that the arrow must be sized and positioned in order to lay precisely over the base component's border line. All other changes are a result of the need for precision sizing/positioning.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@jzempel jzempel requested a review from a team as a code owner May 22, 2024 20:32
@ze-flo
Copy link
Contributor

ze-flo commented May 23, 2024

Bummer. 😢 Probably due to pixel rounding, there's a tiny gap on low-res screen at zoom 100% with inset === 0.

Screenshot 2024-05-23 at 7 57 16 AM

@@ -54,65 +36,77 @@ const animationStyles = (position: ArrowPosition, modifier: string) => {

const positionStyles = (position: ArrowPosition, size: string, inset: string) => {
const margin = math(`${size} / -2`);
const placement = math(`${margin} + ${inset}`);
const placement = math(`${margin} + ${inset} - 1`);
const _position = math(`${size} + 4`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the 4 come from?

Perhaps renaming _position to offset could help better differentiate it from arrow position? 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Where does the 4 come from?

It was the unit needed to keep consistent with previous as it provided the means to bury a bigger arrow into the base element. TBH I'm not super happy about it.

* 3. Arrow positioning on the base element.
*/
return css`
&::before {
${propertyRadius!}: 100%; /* [1] */
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

${propertyRadius!}: 100%; /* [1] */
clip-path: ${clipPath}; /* [2] */
clip-path: ${clipPath}; /* [1] */
${borderCss}; /* [2] */
}

&::before,
&::after {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might not even need an ::after after all! 😉 🙌

Screenshot 2024-05-23 at 7 01 53 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

We need ::after to retain the box-shadow inheritance. Otherwise, the arrow pokes out un-shadowed. Not terrible, but the original designs wanted the shadow to follow the arrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally missed that but after some CSS tweaking I see it now. Thanks!

Screenshot 2024-05-23 at 10 27 29 AM

border-width: inherit;
border-style: inherit;
border-color: transparent;
border: inherit;
background-clip: content-box;
}

&::after {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. ::after might be superfluous.

@jzempel
Copy link
Member Author

jzempel commented May 23, 2024

Bummer. 😢 Probably due to pixel rounding, there's a tiny gap on low-res screen at zoom 100% with inset === 0.

Screenshot 2024-05-23 at 7 57 16 AM

Ugh. Nice find.

@coveralls
Copy link

Coverage Status

coverage: 96.095% (+0.02%) from 96.075%
when pulling f087886 on jzempel/arrow-styles
into 2649a2a on next.

@jzempel
Copy link
Member Author

jzempel commented May 30, 2024

Closing in favor of #1814

@jzempel jzempel closed this May 30, 2024
@jzempel jzempel deleted the jzempel/arrow-styles branch June 7, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants