-
Notifications
You must be signed in to change notification settings - Fork 26
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: LinkButton to use inline TextBody #627
Conversation
341202c
to
d2d1094
Compare
@@ -94,6 +94,7 @@ Wraps the given text in a `<p>` element, for normal content. | |||
| `children` | `PropTypes.node` | ✅ | - | - | | |||
| `title` | `String` | - | - | - | | |||
| `truncate` | `Bool` | - | - | `false` | | |||
| `isInline` | `Bool` | - | - | `false` | |
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.
is this used anywhere?
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.
This is the README of the Text component, I think the props was missing from the documentation...right?
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.
Yes, it was already being used, just not documented.
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.
oooh 🤦♂️
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.
👍
Summary
This PR:
Text.Body
andText.Detail
to include theisInline
prop.Description
There is still a problem with the
LinkButton
rendering adiv
(from theSpacings.Inline
wrapper), which inside ap
is also an invalid DOM nesting. I shall address this in a follow-up PR.