Fluentui: Persona's default icon/initials render with a defined onRenderCoin

[Lego6245]

Environment Information

• __Package version(s)__: Observed on 6.182 and later
• __Browser and OS versions__: Chromium, Windows 10.

Please provide a reproduction of the bug in a codepen:

1. Define a Persona component with an onRenderCoin prop.
2. Do not define an image, so initials or base icon will render instead.

https://codepen.io/anon/pen/VONYVK <- Reproduction of issue.
https://codepen.io/anon/pen/xNebeG <- Expected behavior (but only when an image is provided).

Actual behavior:

The onRenderCoin and the initials/default icon render side by side in the dom.

Expected behavior:

The onRenderCoin is rendered, and no initials/default icon are rendered instead.

Priorities and help requested:

Are you willing to submit a PR to fix? Yes

Requested priority: Normal

[Lego6245]

I did some investigating.

PersonaCoin has two props of interest here, onRenderCoin and onRenderInitials. There's a check to see if initials should be rendered, and if it passes, onRenderInitials is called, either with the user supplied prop or with the default behavior.

A separate check is then made against showUnknownPersonaCoin to determine if the image should render, and then onRenderCoin is called, again with the user provided prop or the default if not provided.

My understanding is the intent is to show the initials or default icon while waiting for the persona to load, then switch over to the image once loaded. This is good behavior, if/when you want the initials to render at all. However, for users where there will be no image provided, if ever, you'll be stuck with the default icon and or initials, rendering side by side with your custom rendered coin.

There's a couple ways you could solve this without fixing this in Fabric:

1. display: none as a style prop for initials.
2. Supply an onRenderInitials prop that returns null.
3. Supply an imageUrl that evaluates truthy.

None of these feel satisfactory. onRenderInitials and onRenderCoin should be mutually exclusive, and in effect, they are. You'll never be able to trigger the state change required to hide the initials/default picture because you won't be able to bind to _onPhotoLoadingStateChange to do so.

[micahgodbolt]

That's funny, I was just noticing this behavior as well. The onRender should certainly replace the original render function.

[aneeshack4]

@micahgodbolt I spoke with @Markionium briefly he said he'll take a look at it tmrw (he's in Oslo so it's currently very late at night for them)

[micahgodbolt]

cool...yeah, shouldn't be a hard fix. just thought it was funny that I ran across the same but on the same day :D

[Markionium]

Hey @Lego6245 ,

I know this is not very straight forward. ~I'm not sure~ (I know you looked at it from your second comment, but perhaps you missed the following detail) if you looked at the source code for the coin but there is a way you're able to achieve what you want on the current version.

If you take your example that is "broken" and add the following to the onRenderPersonaCoin in the custom renderer you created it should do what you'd expect.

if (!personaProps.imageUrl) {
  return null;
}

I know this is totally not intuitive.

Like you mentioned onRenderCoin is always called except for when showUnknownPersonaCoin is set (This unknown persona coin is a variant we have in Outlook that will never show the image even when you pass one).
https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/components/Persona/PersonaCoin/PersonaCoin.base.tsx#L114

The default implementation for on render coin checks if an image exists and if not renders null (like I mentioned above.
https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/components/Persona/PersonaCoin/PersonaCoin.base.tsx#L132-L135

There is no reason why we can't move the if check out to the function that calls onCoinRender. So i'll create a PR for that.

[Lego6245]

@Markionium,

I don't believe that code block fixes the issue. I want to render _my_ coin here. Returning null when imageUrl is not set will instead render the default initials and/or icon. This is explicitly what I do not want to occur in this case. I want to render my custom coin in all cases.

[Lego6245]

I have prepared a new code pen with my intended effect, with your code block, and without your code block to illustrate more clearly :)

I used the display: none trick I proposed above as a way to fix this on the consumer side. I don't think it should be expected for the consumer to have to do this.

https://codepen.io/anon/pen/NVVGox

[Markionium]

I see, yes that makes sense to me. Changing it to not call onRenderInitials and always call onRenderCoin would be a breaking change. I'm ok with doing the change but this could break others who currently rely on the funky behaviour. @micahgodbolt should we fix this non the less and change it to the behaviour @Lego6245 expects? (which would make the most sense to me) If Micah is ok with this change I'll do that instead.

There will be a Persona that would support your case (it's currently in @uifabric/experiments) where you'd be able to pass a coin slot to replace the whole coin. But that doesn't help your current case.

I also found the behaviour I was describing earlier was added on purpose. I'll talk to @mtennoe about this tomorrow.
https://github.com/OfficeDev/office-ui-fabric-react/pull/6106

[mtennoe]

@Markionium and I have synced today. We both agree that the current behavior is not expected. I think what Mark is suggesting above makes sense, even if it is a breaking change (as the original behavior is unintended). Its up to @micahgodbolt though

[micahgodbolt]

Quick clarification. The personaCoin includes the image/initials, as well as the activity status icon. Is the goal to get an onRender function around just the circular image/initials? Or are you looking to have the render function around the entire coin component (presence included).

In my head, onRenderCoin should render the entire PersonaCoin (image/initials/presence). This would mean elevating it up to the persona itself. This DOES feel like quite the breaking change though, so we might consider onRenderPersonaCoin for that.

[micahgodbolt]

i.e. onRender should simply replace the default rendering of an element. All of the logic of WHAT gets rendered should stay the same.

It appears that the problem is that the PersonaCoin's _onRenderCoin includes a check for !imageUrl before actually rendering. So if you replace the _onRenderCoin you don't get that check. That same !imageUrl is also used to determine if the initials should be rendered. This is why you are getting both.

One possible solution is to test for custom onRenderCoin function in addition to !imageUrl

    const shouldRenderInitials = Boolean(
      !this.props.onRenderCoin && !this.state.isImageLoaded && ((showInitialsUntilImageLoads && imageUrl) || !imageUrl || this.state.isImageError || hideImage)
    );

In general, all of this logic is pretty messy. A nice state diagram would be really helpful.

[aneeshack4]

@Markionium @mtennoe Just following up on this since there hasn't been activity for a couple days - are y'all planning on implementing this change that Micah's suggesting?

[mtennoe]

@aneeshack4 - Yup, we will handle it!

[mtennoe]

How urgent do we believe this is btw? Considering capacity in my team, can we wait until next week? (considering how long this has been there, I would say yes)

[micahgodbolt]

Yeah, there are workarounds, so it's not blocking.

[msft-github-bot]

:tada:This issue was addressed in #9725, which has now been successfully released as [email protected].:tada:

Handy links:

• Release Notes
• Official NPM Release
• Test the release with Developer Demo Site