Skip to content

Conversation

ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Aug 7, 2024

Description

This PR refines the arrowStyles logic to output deterministic arrow dimensions.

Detail

  • Simplifies Tooltip styles to avoid relying on arrow inset
  • Ensures the rendered height of the arrow matches with the size argument

For this example, ArrowStylesStory was modified to display a box-shadow matching the arrow size input.

box-shadow: ${p => p.hasBoxShadow && `0 0 0 ${p.size}px lavender`};

This preview shows the impact of changing the size and inset inputs. The arrow was recolored to better show its dimensions.

Screen Recording 2024-08-07 at 10 18 05 AM

Notice that the tip of the arrow ends where the drop shadow ends.

As a result, tooltip arrow rendered sizes match with V8:

Screenshot 2024-08-07 at 12 46 33 PM

Arrows checklist

  • Renders Tooltips correctly
  • Renders `ArrowStyles correctly
  • Renders Legacy Menu correctly
  • Renders Menu correctly

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
  • 📝 Thoroughly tested in Chrome, Firefox, Safari, and Edge on retina and non-retina displays

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

Arrows for both Menu and legacy Menu are appearing too large compared to v8.

const SIZE = ['2px', '4px', '6px', '8px', '10px', '1em'];

SIZE.forEach(size => {
console.log('size:', size);
Copy link
Member

@jzempel jzempel Aug 8, 2024

Choose a reason for hiding this comment

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

Remove console.log. Please check eslint config – no-console should be getting flagged for spec files.

@ze-flo
Copy link
Contributor Author

ze-flo commented Aug 8, 2024

Arrows for both Menu and legacy Menu are appearing too large compared to v8.

@jzempel Fixed in 29f8680.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

From what I can tell everything looks right when comparing to V8 Storybook (Menus + Modals + Tooltips). 💯

I also like how consistent the stroke of the arrow is. If I'm not mistaken it's changed only slightly enough to be nearly perfectly on the same width as the box border. 😁

V8
Screenshot 2024-08-08 at 12 57 18 PM

V9
Screenshot 2024-08-08 at 12 57 21 PM

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

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

🙌

@ze-flo ze-flo merged commit 63ac08b into next Aug 8, 2024
@ze-flo ze-flo deleted the ze-flo/arrow-styles-fix branch August 8, 2024 18:59
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.

3 participants