-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Item): actually make content
a shorthand prop for ItemContent
#4118
base: master
Are you sure you want to change the base?
Conversation
As already documented.
Codecov Report
@@ Coverage Diff @@
## master #4118 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 180 180
Lines 3241 3246 +5
=======================================
+ Hits 3233 3238 +5
Misses 8 8
Continue to review full report at Codecov.
|
contentShorthandValue === undefined && | ||
[description, extra, header, meta].some((prop) => !_.isNil(prop)) | ||
) { | ||
contentShorthandValue = {} |
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.
Only setting this when content
is strictly undefined
here and letting ItemContent.create
handle everything else, because it allows the user to still use null
(or a boolean) to prevent rendering when one of the other shorthands is set. Maybe true
should also be included though?
/> | ||
{ItemContent.create(contentShorthandValue, { | ||
autoGenerateKey: false, | ||
overrideProps: { description, extra, header, meta }, |
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.
Alternatively you could use defaultProps
here, but I think it makes sense for them to override because they're more 'specific'.
It says it is in the docs, but actually
Item
currently just forwards thecontent
prop toItemContent
like a typical content prop. This PR will allow use ofas
,className
andverticalAlign
via shorthand, and is especially useful when providing theitems
array shorthand toItemGroup
.With this PR nothing changes for people who were using a primitive value, but it might be a breaking change if people were supplying an element or array, they would need to change it to
content={ content: <element /> }
or similar to match previous behaviour.Also with this change
ItemContent
will no longer unconditionally render, at least one of the shorthands has to be present.