-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Persona - onRenderCoin prop #3799
Persona - onRenderCoin prop #3799
Conversation
@autobind | ||
private _onRenderCoin(props: IPersonaProps, size: number): JSX.Element { | ||
if (props.onRenderCoin) { | ||
console.log(size); |
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.
Probably wasn't meant to be included in the commit.
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.
Yes, I'll clean that up. I was attempting to make a slightly better example to show. If you have any ideas on that let me know.
if (props.onRenderCoin) { | ||
console.log(size); | ||
return( | ||
<div> |
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.
Does the Coin need to be wrapped in these div
s?
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 think the change I just committed is a bit cleaner. The div can be removed if a null return from _onRenderCoin is allowed which would just print nothing.
shouldStartVisible={ imageShouldStartVisible } | ||
onLoadingStateChange={ this._onPhotoLoadingStateChange } | ||
/> | ||
{ this._onRenderCoin( this.props, size ) } |
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.
onRenderCoin
could be refactored a bit. IRenderFunction
should take props
and the default render as parameters. See Tooltip's onRenderContent
for example
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.
Ah nice! yeah that's much cleaner thank you so much!
@@ -5,7 +5,8 @@ import { | |||
divProperties, | |||
getInitials, | |||
getNativeProps, | |||
getRTL | |||
getRTL, | |||
IRenderFunction |
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.
IRenderFunction
isn't used
Please run |
@@ -10,4 +10,7 @@ | |||
.example-label { | |||
margin: 10px 0; | |||
} | |||
.custom-example-coin img { |
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 don't typically use the kebab-case naming convention for our classes, can you change this one and the one above to camelCase?
As @jordandrako pointed out, the Screener regressions are on the master branch and are unrelated to this change. |
…c-react into persona-coin-or-1094
…oft#3799) onRenderCoin prop that controls the image coin - with example
Pull request checklist
$ npm run change
Description of changes
Added an onRenderCoin prop to Persona to allow for passing in or JSX for expanded functionality. Pretty basic set up right now and I am open to suggestions on how to polish this into a solid feature of this component.
Focus areas to test
Open to suggestions