-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17964 +/- ##
==========================================
- Coverage 84.04% 84.03% -0.02%
==========================================
Files 408 408
Lines 14470 14473 +3
Branches 4660 4662 +2
==========================================
+ Hits 12162 12163 +1
- Misses 2142 2145 +3
+ Partials 166 165 -1 ☔ 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.
![Definition tooltip (auto align_ true)](https://private-user-images.githubusercontent.com/43969356/383995969-e2864b3f-1a42-4b4a-9636-230175087114.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2ODMyNTYsIm5iZiI6MTczOTY4Mjk1NiwicGF0aCI6Ii80Mzk2OTM1Ni8zODM5OTU5NjktZTI4NjRiM2YtMWE0Mi00YjRhLTk2MzYtMjMwMTc1MDg3MTE0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE2VDA1MTU1NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTY3ZDBhZWY2NWFkYjBlODBlZThjODA2MDU4NmZkMzRiY2ZkZTkxZDQ4ZjEwNGNkMWFiMDBjOGI5OWQwN2EzNDkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.0LCV9z59niDH-kzfI5dDAEEiQbm8m5aqEUPDOGJTBC8)
- 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. |
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.
Hmm.. I think it's taking the tooltip to the position we are assigning, not sure about what logic it's applying to calculate the arrow position. Let me check how we are showing for other tooltip/popover with large target text. |
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.
A few small things I saw in the comments below. Does this fix the root issue of DefinitionTooltip's misplaced caret with long text without having to turn on autoAlign
?
@preetibansalui just wanted to confirm this can be fixed without having to turn on |
Closes #17848
Definition Tooltip breaks with large text
Changelog
align
value tobottom
instead ofbottom-start
Testing / Reviewing