-
Notifications
You must be signed in to change notification settings - Fork 55
fix(factories): handle ReactElement as primitive #1513
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1513 +/- ##
==========================================
- Coverage 71.24% 71.23% -0.02%
==========================================
Files 851 851
Lines 7021 7032 +11
Branches 1999 2004 +5
==========================================
+ Hits 5002 5009 +7
- Misses 2013 2017 +4
Partials 6 6
Continue to review full report at Codecov.
|
docs/src/examples/components/Avatar/Variations/AvatarExampleImageCustomization.shorthand.tsx
Outdated
Show resolved
Hide resolved
}, | ||
overrideProps: props, | ||
}), | ||
) |
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.
I am still disappointed with this.
As I see we used our own components in the wrong way, I expect that we can reuse Image
and Icon
components, but should we reuse Label
component in this case? Not sure 🌵
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 the only place where we implicitly pass shorthand of a shorthand? This will be issue in every case we have like this. Meaning, we have shorthand for something that is shorthand in another element..
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.
We have this only in Dropdown
components. I feel that it will be easier to have some custom components instead of this composition
@@ -441,7 +441,7 @@ export default class Popup extends AutoControlledComponent<PopupProps, PopupStat | |||
const popupContentAttributes = | |||
accessibility.focusTrap || accessibility.autoFocus ? {} : popupWrapperAttributes | |||
|
|||
const popupContent = Popup.Content.create(content, { | |||
const popupContent = Popup.Content.create(content || {}, { |
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.
If we will pass false
or null
to this slot we will render nothing and it will break Popper
and Ref
components that expects children. I want to consider refactoring of this place more deeply.
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.
Are you going to create a separate issue for that?
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.
#1630 👍
64f6d26
to
d63151f
Compare
d63151f
to
9f7a429
Compare
@@ -62,9 +62,9 @@ class Label extends UIComponent<WithAsProp<LabelProps>, any> { | |||
static propTypes = { | |||
...commonPropTypes.createCommon({ color: true }), | |||
circular: PropTypes.bool, | |||
icon: customPropTypes.itemShorthand, | |||
icon: customPropTypes.itemShorthandWithoutJSX, |
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.
Maybe we should rethink how we create these custom prop types - the user here must know that the image and icon are this special type...
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.
Can you please provide more info? We already have a custom propTypes for them...
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.
My point was, the user must know that the Icon
and Image
have different type then the other shorthands, and this is very prone to mistake in the future... Maybe we can make the component type param to the item shorthand and have one centralized place where this logic will be implemented. Other then this don't have any better idea.. What do you think?
docs/src/examples/components/Chat/Types/ChatExample.shorthand.tsx
Outdated
Show resolved
Hide resolved
Co-Authored-By: Marija Najdova <mnajdova@gmail.com>
/> | ||
``` | ||
|
||
### Customizing rendered tree |
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.
We should also add new use case now - completely replace slot content.
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.
Actually, the example in this section does required thing. I removed a paragraph that doesn't have any sense now, 1fbc89a
Co-Authored-By: Miroslav Stastny <mistastn@microsoft.com>
Co-Authored-By: Miroslav Stastny <mistastn@microsoft.com>
packages/react/src/components/Dropdown/DropdownSelectedItem.tsx
Outdated
Show resolved
Hide resolved
yarn.lock
Outdated
@@ -9,7 +9,27 @@ | |||
dependencies: | |||
"@babel/highlight" "^7.0.0" | |||
|
|||
"@babel/core@^7.1.0", "@babel/core@^7.2.2", "@babel/core@^7.3.4": | |||
"@babel/core@^7.1.0", "@babel/core@^7.3.4": |
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.
Can you check again why we have changes in the yarn.lock and if we have new dependencies that require OSS scan?
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.
I reverted these changes because I don't why they happened 🤔 There are no any changes in any package.json
BREAKING CHANGES
This PR changes how
React.Element
s are handled in shorthands, this change affects all components. To replace elements consider to userender()
callback.Before
React.Element
replaced matching slot.After
React.Element
will be inside matching slot.To keep existing behavior with replacement consider to use Render Callback to completely replace contents of slot. However, in the most case enough to use Object Shorthand.
See Shorthand props docs for more details.
Problem
Our component factories handles primitives (
string
,number
) andReact.Element
s in different ways. This issue comes from Semantic UI React where it was a single way to modify rendered element in matching slot. But, we haverender()
callback that covers that purpose.The most common use case is over-complicated and disguised from consumer:
We can't always apply props properly to passed element, it's a black box for us:
Solution
When passing JSX, it should be a child of the slot component, this is the more intuitive behavior:
I.e. we will handle
React.Element
in the same way asstring
, so the approach will be consistent.Shorthand without JSX
Shorthands like
icon
s andimage
s can't renderchildren
and have mapping to other props thancontent
. That's why we should handle these cases separately and rendernull
in that case. Factories were updated to handle a newallowJSX
param in config.