-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: definition tooltip breaks with large text #17964
base: main
Are you sure you want to change the base?
fix: definition tooltip breaks with large text #17964
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17964 +/- ##
==========================================
+ Coverage 84.17% 84.18% +0.01%
==========================================
Files 408 408
Lines 14435 14432 -3
Branches 4640 4691 +51
==========================================
- Hits 12150 12149 -1
+ Misses 2121 2119 -2
Partials 164 164 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@preetibansalui Other than that, the fix looks good to me. |
That autoAlign is an optional prop so depends on user in what condition they want to make it on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@preetibansalui When the autoAlign
prop is set to True, the "bottom" alignment looks fine, but the "bottom-left" and "bottom-right" caret is visually off. I also think that if the user is choosing a "bottom-left" or "bottom-right" alignment, the tooltip container needs to be vertically flush with the definition text depending on which alignment is being used.
- Also unrelated, but there seems to be some more spacing alignment bugs with some of the alignment options that is unrelated to this PR, and I can make a separate issue for that in our repo.
Hey @laurenmrice , Sorry little confusion here. In that case will it be similar for Also, do we have to tackle it in |
When the tooltip container appears, it needs to align vertically flush with the beginning (bottom-left) or end (bottom-right) of the definition tooltip text on the page. If adding padding from left/right helps resolve that then yes.
Yes, if that is possible to do for the
Would that be a separate PR from this? I see some spacing issues in the Tooltip component, but it's mostly around where the caret is being placed on the tooltip container. |
Closes #17848
Definition Tooltip breaks with large text
Changelog
align
value tobottom
instead ofbottom-start
Testing / Reviewing