Skip to content

Conversation

ze-flo
Copy link
Contributor

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

Description

Fixes a regression created in #1882

Screenshot 2024-08-21 at 12 16 43 PM

Detail

  • Refines defaultInset to better render on retina and regular monitors
  • Adds ability to shift the arrow placement alongside the edge of the parent container. This addresses the case when a parent container is too short for be supported by the default arrow placement logic.

For example:

The arrow of a small tooltip using end-bottom placement will extend past the container's boundaries because of the border-radius

Before

Screenshot 2024-08-21 at 12 10 51 PM

After

Screenshot 2024-08-21 at 12 12 12 PM

Checklist

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

@ze-flo ze-flo changed the title fix(theming): fix arrow position when using end-bottom placement fix(tooltips): fix arrow position when using end-bottom placement Aug 21, 2024
Comment on lines 80 to 83
arrowSize = margin;
if (['left-start', 'left-end', 'right-start', 'right-end'].includes(placement)) {
arrowShift = '-4px';
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
arrowSize = margin;
if (['left-start', 'left-end', 'right-start', 'right-end'].includes(placement)) {
arrowShift = '-4px';
}
if (['left-start', 'left-end', 'right-start', 'right-end'].includes(placement)) {
arrowShift = '-4px';
} else {
arrowShift = margin;
}

[nit] prefer clarifying assignment

if (size === 'small') {
arrowSize = margin;
if (['left-start', 'left-end', 'right-start', 'right-end'].includes(placement)) {
arrowShift = '-4px';
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to tie this to the theme with

arrowShift = `-${theme.borderRadii.md}`;

to better indicate the shifting rationale and respond to theming customizations that knock out the borders?

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.

Nice!

@ze-flo ze-flo marked this pull request as ready for review August 22, 2024 17:32
@ze-flo ze-flo requested a review from a team as a code owner August 22, 2024 17:32
@ze-flo ze-flo merged commit 61b174f into main Aug 22, 2024
@ze-flo ze-flo deleted the ze-flo/fix-arrow-positioning branch August 22, 2024 18:02
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