Annotate Players in ViewReport

[GreenAsJade]

Fixes Moderators losing track of who is who

Proposed Changes

• Have Player provide a context that it can be rendered in to enable it to annotate participants in a report.
• Have ViewReport use that context.

[github-actions]

Uffizzi Preview deployment-55541 was deleted.

[anoek]

This feels wrong, we shouldn't be spreading the context logic over to a generic component like Player I don't think. How about instead we put that context stuff up in the report code and then pass "reported" in for the ui_class for Player?

[GreenAsJade]

Here's my case for doing it this way :)

"Player" is a component that knows how to render Players in various contexts.

One of those is "in a report context". It makes a context provider available to components that feel they are "in a report context", and lets those components describe which two players have the context-specific roles.

Once that's done neither component has to worry anymore about remembering to (or even being able to) think about what is the current' player's role. Instead every instance of Player within that context is "magically" annotated if the Player in question is one of the relevant ones.

Note how the ReportedGame is made of a hiearachy of components, many of which have Player components instantiated in them, and every single one is correctly annotated, in the screenshot above, without editing any code in any of them...

(Actually, you can't see it in that screenshot, but even GameLog when instantiated inside ReportedGame has the players annotated ... and it doesn't have them annotated when instantiate in the Game context... which makes sense because in that context there is no concept of reporter and reported. )

[anoek]

"Player" is a component that knows how to render Players in various contexts.

Does it? I always viewed it as a generic component that didn't care where it was rendered, it just rendered the bits of a player name that you told it to.

That changes fundamentally with this. Now part of the rendering code for a view is pushed down into a generic component, feels like that is a pattern that could get out of hand very quickly.

However, I could see how getting the data to all the right places might get painful. Let me ponder, will probably merge as is but want to digest if there's a cleaner way.

[GreenAsJade]

Does it? I always viewed it as a generic component that didn't care where it was rendered, it just rendered the bits of a player name that you told it to.

I guess that is true.

I tried to design this extension in a "elegant" way, by having the "context provider" owned by Player - it comes from Player and works in the way Player determines, along with the implementation detail of how Players in that context should be rendered.

So Player says "if you tell me that you are setting up this context that I have provided to you, I will interpret all Players inside that context in the special way that I have set up for this kind of thing".

Now part of the rendering code for a view is pushed down into a generic component

I'm not seeing this. A Player component is responsible for rendering the Player - it only has Player rendering code.

The only change here is the way in which it gets the information on how to treat the specific Player ... not only listening to props to find out, but also listening to information coming in on a context provider that it handed out...

[anoek]

Now part of the rendering code for a view is pushed down into a generic component

I'm not seeing this. A Player component is responsible for rendering the Player - it only has Player rendering code.

ehh, the Player.styl now has code that only takes effect when in the #ViewReport element and we have a special ViewReportContextType type that is, as indicated by the variable name, clearly only applicable for the ViewReport view.

I guess though it probably wouldn't have felt so wrong if it was spun as a more generic thing, as opposed to something that only works when in ViewReport, so in that light maybe it's fine, and maybe if we find another place where that pattern is a good fit we make it more generic or something.

[GreenAsJade]

Drat, I should have called the context something less similar to a page name :)

I totally agree that the #ViewReport is Player.styl is regrettable.

Let me think about that.

[GreenAsJade]

I think that there should be a contract about the styling. The component that does instantiates the context

<ViewReportContext.Provider...

should also keep the contract by putting a className that Player is expecting around the context as well, rather than Player looking for #ViewReport (which is a specific special component).

I'll see about making it feel more like a generic pattern and then we'll see how it strikes you...

... and in the mean time if you can think of a different way of ensuring that "all Players within this page get annotated" that'd be cool!

[anoek]

... and in the mean time if you can think of a different way of ensuring that "all Players within this page get annotated" that'd be cool!

I think I've come to terms with Context's being the best way to achieve that, everything else is pretty ugly. But let's at least pretend it's generic, and who knows it might be. At least we'll know where to add similar context stuff if we have need. For instance, one could imagine a tournament creator/director getting set in the context of a game or tournament page so all chats from them have a little TD next to them or something like.

It think for the style thing we can probably just remove the #ViewReport qualification, presuming the align-items center thing doesn't break Player normally. (If it does, we can do &.reporter, &.reported { align-items: center } and that should fix it too I'd think)

[GreenAsJade]

It felt safer to have a deal with the context consumer to specifically contain effects of the annotation styling.