add onRenderDescription to TextField#4588
add onRenderDescription to TextField#4588dzearing merged 7 commits intomicrosoft:masterfrom chrismohr:onRenderDescription_for_TextField
Conversation
| } | ||
|
|
||
| @autobind | ||
| private _onRenderDescription(props: ITextFieldProps): JSX.Element | null { |
There was a problem hiding this comment.
we've moving away from autobind to arrow functions.
There was a problem hiding this comment.
ok, I've made the change
| description?: string; | ||
|
|
||
| /** | ||
| * Optional custom renderer for the description |
There was a problem hiding this comment.
description [](start = 38, length = 11)
nit: . at the end of the sentence.
| /** | ||
| * Optional custom renderer for the description | ||
| */ | ||
| onRenderDescription?: IRenderFunction<ITextFieldProps>; |
There was a problem hiding this comment.
onRenderDescription [](start = 2, length = 19)
Please add this along with the rest of the onRender APIs below
There was a problem hiding this comment.
Added periods to the descriptions of all onRender.... functions
| return null; | ||
| } | ||
|
|
||
| private _onRenderDescription = (props: ITextFieldProps): JSX.Element | null => { |
There was a problem hiding this comment.
null [](start = 73, length = 4)
undefined
There was a problem hiding this comment.
@manishgarg1 I've updated both _onRenderDescription and _onRenderLabel.
A bit different than you suggested though. Rather than replacing null with undefined, I was able to remove null from the return type by moving the guard into the render method.
(I wasn't sure for the motivation behind your suggestion, so please let me know if I missed the point.)
The reason I did it this way was because changing null to undefined on the return type resulted in the following error at the onRenderLabel invokation, which I don't yet understand.
[ts] Cannot invoke an expression whose type lacks a call signature. Type 'IRenderFunction<ITextFieldProps> | ((props: ITextFieldProps) => Element | undefined)' has no compatible call signatures.
const onRenderLabel: IRenderFunction<ITextFieldProps> | ((props: ITextFieldProps) => JSX.Element | undefined)
There was a problem hiding this comment.
@manishgarg1 On further consideration, I don't think what I did will work for us. We'd want the provided onRenderDescription to be invoked even when description is absent. I've reverted this last change.
If you still think null should be changed to undefined, can you please help me understand the motivation? And suggest a course of action given the error that I encountered above?
| if (description) { | ||
| return (<span className={ css('ms-TextField-description', styles.description) }>{ description }</span>); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
The following code will do the same
return description && <span....>
There was a problem hiding this comment.
Slight challenge here was that we'd have to add string to the return type, because of falsey empty strings.
|
i wonder if it would be simpler to allow |
|
Yeah, I was thinking about that option as well. I think for consistency an onRender isn't a bad choice though. |
|
👍 |
|
Yeah, I'm open to either. Did onRender to match what was there for label, prefix etc. |
| } | ||
|
|
||
| private _onRenderDescription = (props: ITextFieldProps): JSX.Element | null => { | ||
| if (props.description) { |
There was a problem hiding this comment.
could we update this to be able to call the onRenderDescription prop even if the description prop is not provided? i think for our use case we'd like our wrapping TextField component to provide onRenderDescription without providing description since our description will allow JSX and cannot be passed in to the Fabric description
There was a problem hiding this comment.
ah i see you already implemented this change within render
|
@micahgodbolt, @dzearing, since I haven't heard back from @manishgarg1 yet, can one of you review/approve please? |
5 participants
Pull request checklist
$ npm run changeDescription of changes
Add onRenderDescription to TextField. Since description only takes a
string, this allows us to render a description with additional markup, like a link for example.(This pr replaces #4576)