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(Item): actually make content a shorthand prop for ItemContent #4118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noinkling
Copy link
Contributor

@noinkling noinkling commented Nov 23, 2020

It says it is in the docs, but actually Item currently just forwards the content prop to ItemContent like a typical content prop. This PR will allow use of as, className and verticalAlign via shorthand, and is especially useful when providing the items array shorthand to ItemGroup.

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.

@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #4118 (a8a317c) into master (c828368) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4118   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files         180      180           
  Lines        3241     3246    +5     
=======================================
+ Hits         3233     3238    +5     
  Misses          8        8           
Impacted Files Coverage Δ
src/views/Item/Item.js 100.00% <100.00%> (ø)
src/views/Item/ItemContent.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c828368...a8a317c. Read the comment docs.

contentShorthandValue === undefined &&
[description, extra, header, meta].some((prop) => !_.isNil(prop))
) {
contentShorthandValue = {}
Copy link
Contributor Author

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 },
Copy link
Contributor Author

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'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants