Include image data for person component context

[miqh]

Closes #167

Hello,

This is a minor fix aimed at addressing #167.

The change is simple enough. However, I dug a little deeper to verify the change and I think there's a separate issue with the component template rendering pipeline which causes custom template definitions to not work.

Specifically, something feels a little off to me with this block:

microsoft-graph-toolkit/src/components/templatedComponent.ts

Lines 91 to 97 in 3f7814c

	for (let i = 0; i < this.children.length; i++) {
	if (this.children[i].slot === slotName) {
	return html`
	<slot name=${slotName}></slot>
	`;
	}
	}

For the Person component with a custom template, I observed a <div slot="default" data-generated="template"> being appended on the initial render (i.e. no person data yet). Once person data becomes available, the above block of code appears to short-circuit the component re-render. This means even though person context data is available, none of that will be used to generate an updated template view.

I found that replacing the immediate return with this.removeChild(this.children[i]); looks like it fixes this problem. The idea here is that the previous slot element rendered gets removed and re-appended with the freshly rendered template. However, I didn't include this change because it seems like it might break something I'm unaware of. Happy to discuss further on the latter.

[nmetulev]

Thanks for the fix @miqh. Looks good and can be merged

As for the other issue, that looks like the cause of #165. I think your solution is correct, bit it can be improved to still avoid having to render templates that are already rendered while re-rendering when context has changed.

We can also improve on using a loop to iterate through all the children if we keep a reference to the slots in a dictionary.

private renderedTemplates: any = {};

and replacing the linked code with something like

if (this.renderedTemplates[slotName]) {
  const currentSlot = this.renderedTemplates[slotName];
  const currentContext = currentSlot.dataContext;
  if (equals(currentContext, context)) {
    return html`
      <slot name=${slotName}></slot>
    `;
  } else {
    this.removeChild(currentSlot);
    this.renderedTemplates[slotName] = null;
  }
}

Where equals() does an equality check of the data context to ensure the data context has changed

Let me know if this is also something you want to take on.

I will go ahead and merge this.

[miqh]

@nmetulev, my pleasure. ☺️

Ah, caching rendered templates as you suggested appears to more performance-friendly approach. I'd neglected to think about the cost of constantly altering the DOM.

I'm happy to implement these follow-up changes, but I just had a few concerns if you don't mind clarifying:

• With the context object diffing function (i.e. equals() in your proposed snippet), do you think a simple deep comparison implemented locally is sufficient? Otherwise, if you anticipate more complex items, such as Set or RegExp, being made available in the context, it might be better to adopt a dependency which does the deep compare thoroughly.
• I can't locate any existing tests which attempt to exercise the core component classes. It'd be nice to bolt some in to catch regressions because it seems like quite an important central piece of common code. Perhaps the APIs are too in flux at the moment?

[nmetulev]

Let's move the discussion over to #165 to keep it relevant to the issue.