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: arrow breaking in AILabel popover #17982

Conversation

preetibansalui
Copy link
Contributor

@preetibansalui preetibansalui commented Nov 6, 2024

Closes #17965

Arrow looks breaking in the AI explainability popover in case of small icons.

Changelog

added alignmentAxis in offset for popover.

Changed

passing a prop from AILabel to Toggletip component in case of 'XS', '2xs' or 'mini' icon.

Testing / Reviewing

  1. Go to Storybook > AILabel > Default
  2. hover on AI Label, the popover should not looks breaking.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit fa64469
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/6746bf8d547c8e0008dea958
😎 Deploy Preview https://deploy-preview-17982--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 1548b8f
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/672b0e5174cffd0008b30df7
😎 Deploy Preview https://deploy-preview-17982--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1548b8f
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/672b0e519b26c500080b6c27
😎 Deploy Preview https://deploy-preview-17982--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit fa64469
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6746bf8dd7ea790008e8d5f8
😎 Deploy Preview https://deploy-preview-17982--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6268c8a
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/672b0f7145bd4900086786a6
😎 Deploy Preview https://deploy-preview-17982--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 6, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fa64469
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6746bf8dd6c6120008fa57d6
😎 Deploy Preview https://deploy-preview-17982--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.36%. Comparing base (0da5e37) to head (fa64469).
Report is 51 commits behind head on main.

Files with missing lines Patch % Lines
packages/react/src/components/Popover/index.tsx 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17982      +/-   ##
==========================================
- Coverage   83.36%   83.36%   -0.01%     
==========================================
  Files         404      404              
  Lines       14344    14350       +6     
  Branches     4646     4655       +9     
==========================================
+ Hits        11958    11963       +5     
+ Misses       2225     2224       -1     
- Partials      161      163       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

The caret looks like its 1px too low.
image

@preetibansalui preetibansalui requested a review from a team as a code owner November 13, 2024 09:31
@preetibansalui
Copy link
Contributor Author

The caret looks like its 1px too low. image

Thanks Anna for noticing this. It was caused by recent renaming of slug to ai-label, which is fixed now.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Member

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +266 to +267
alignmentAxis: alignmentAxisOffset,
mainAxis: popoverDimensions?.current?.offset,
Copy link
Member

Choose a reason for hiding this comment

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

This isn't something blocking this PR and doesn't need changed, but one concern I just mentioned a comment on #17003 (comment) is that as we add more of these config values, they bloat the interface/api of these components. Especially when we haven't identified a use case from a consumer for this, it's just something we need internally for our use w/ AILabel.

The mainAxis offset is already pulled from a custom property instead of a prop value. Doing this for other values as well might be one way to scale these types of config values without having to have a prop for each one. Could be used in tandem w/ however we end up providing a way for consumers to config floating-ui from anywhere in their react tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood your point, I added a prop here instead of some calculations so that it could be changed to different cases as well which might come in future as we have now for AILabel.

@tay1orjones tay1orjones added this pull request to the merge queue Dec 17, 2024
Merged via the queue into carbon-design-system:main with commit 1cf7200 Dec 17, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Arrow looks breaking in the AI explainability popover
5 participants