[StepLabel] Introduce StepIconComponent property #13003
Conversation
oliviertassinari
added
new feature
| /** | ||
| * The React Element to render in place of the [`StepIcon`](/api/step-icon) element. | ||
| */ | ||
| StepIconComponent: PropTypes.oneOfType([ |
There was a problem hiding this comment.
You expect a component here? string, object or function are sufficient in that case.
| icon={icon} | ||
| {...StepIconProps} | ||
| /> | ||
| ) : ( | ||
| <StepIcon | ||
| completed={completed} | ||
| active={active} | ||
| error={error} | ||
| icon={icon} | ||
| {...StepIconProps} | ||
| /> |
There was a problem hiding this comment.
The same props are applied to both paths. Why not just let StepIconComponent default to StepIcon?
|
To fix CI run in the workspace root:
|
| @@ -78,6 +78,7 @@ function StepLabel(props) { | |||
| last, | |||
| optional, | |||
| orientation, | |||
| StepIconComponent = StepIcon, | |||
| StepIconProps, | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -96,13 +97,13 @@ function StepLabel(props) { | |||
| )} | |||
| {...other} | |||
| > | |||
| {icon && ( | |||
| {(icon || StepIconComponent) && ( | |||
| <span | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@eps1lon Thanks for all the input. I'm a bit uncomfortable with this proposal, as similar props of StepLabel are using The simplest way to achieve the same behaviour would be to clone the icon to inject props in the StepIcon component, but that would introduce a slight behavioural change. What's your opinion ? |
There was a problem hiding this comment.
The simplest way to achieve the same behaviour would be to clone the icon to inject props in the StepIcon component, but that would introduce a slight behavioural change.
The idea here is to provide a high level of flexibility. I think that using a component is a great to achieve it. A render property would be another way but requires a global design. We try to use cloneElement as few as possible, it's generally speaking an anti-pattern.
| @@ -184,6 +185,10 @@ StepLabel.propTypes = { | |||
| * @ignore | |||
| */ | |||
| orientation: PropTypes.oneOf(['horizontal', 'vertical']), | |||
| /** | |||
| * The React Element to render in place of the [`StepIcon`](/api/step-icon) element. | |||
There was a problem hiding this comment.
It's a component not an element
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
StepIcon
branch
2 times, most recently
from
8f955cd
to
5521768
Compare
Add StepIconComponent tests propTypes validation - StepIconComponent can be a function/class or a ForwardRef
oliviertassinari
force-pushed
the
StepIcon
branch
from
5521768
to
351a4fe
Compare
|
@semos It's a great first pull request on Material-UI 👌🏻. Thank you for working on it! |
* Add StepIconComponent prop to use customized icon Add StepIconComponent tests propTypes validation - StepIconComponent can be a function/class or a ForwardRef * update api doc * prettier * update doc * consolidation * let's merge
Add a StepIconComponent prop in StepLabel to overwrite StepIcon standard behaviour with the provided component.
The component will be instantiated with the completed/active/error props.
Unit tests are OK.
fix #12987