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(TooltipDefinition): update children PropType #7437

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Dec 10, 2020

Closes #6484

This PR changes the TooltipDefinition's children prop type to support nodes instead of restricting it to strings (alternatively, we can insert a span that wraps the children if we want to keep the proptype definition as string). This will allow users to truncate the tooltip trigger text if necessary with CSS, and our existing guidelines dictate that the definition tooltip should be used for small amounts of text

Changelog

Changed

  • TooltipDefinition children proptype definition

@netlify
Copy link

netlify bot commented Dec 10, 2020

✔️ Deploy preview for carbon-elements ready!

🔨 Explore the source changes: ebce032

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-elements/deploys/6001ea28761d410007b44981

😎 Browse the preview: https://deploy-preview-7437--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 10, 2020

✔️ Deploy preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: ebce032

🔍 Inspect the deploy logs: https://app.netlify.com/sites/carbon-components-react/deploys/6001ea2879ae3b0008c2426b

😎 Browse the preview: https://deploy-preview-7437--carbon-components-react.netlify.app

@emyarod emyarod requested a review from a team as a code owner December 10, 2020 20:16
@joshblack
Copy link
Contributor

If I understand correctly, this is allowing truncation of the text that triggers the tooltip? If so, do we need to worry about how someone could see what the truncated text itself is? Seems like we solved that in the past with tooltips or (unfortunately) title, but if this itself has a tooltip it seems like it would conflict.

Let me know if I'm misunderstanding, just was curious 🤔

@emyarod
Copy link
Member Author

emyarod commented Dec 15, 2020

we wouldn't be able to control the truncation logic since that would all be on the application's side. this change is just for the proptype definition since the user is technically already able to truncate the tooltip trigger

Copy link
Contributor

@andreancardona andreancardona 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!

wait..

@emyarod is this still pending a question?

@kodiakhq kodiakhq bot merged commit e34214c into carbon-design-system:master Jan 15, 2021
@emyarod emyarod deleted the 6484-tooltipdefinition-children-proptype branch January 19, 2021 16:18
@emyarod emyarod mentioned this pull request Jan 22, 2021
46 tasks
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.

[Tooltip/TooltipDefinition] Support for hover or truncation with ellipses
3 participants