Allow EuiContextMenuPanels to update when their children changes

[chandlerprall]

Fixes #674. EuiContextMenu accepts panel shapes with either items or content, passing that data through to EuiContextMenuPanel. The panel component's shouldComponentUpdate previously only checked if it was in a transitioning state or if its items had changed, and had no logic to allow it's children (the EuiContextMenu's content) to update.

This PR changes EuiContextMenuPanel's shouldComponentUpdate to return true if any children were passed. This pushes a responsibility onto that child content to self-manage if/when there are performance considerations when rendering that content (which is better practice anyway).

[cjcenizal]

🎸 THANK YOU FOR ADDING TESTS! This solution looks great. Really nice analysis in the originating issue. I just had a couple comments for your consideration.

[cjcenizal]

I don't think this comment really adds much more information than the code already provides, because it's describing what the code does instead of why it does it. I think we can make this comment more helpful by explaining the intention here, like you did in the PR description, e.g.

// Allow children to re-render.

[cjcenizal]

Do we need this Promise? I tested it without it and the test seemed to work normally. Maybe I'm missing something?

Also, I found this test a little difficult to follow because of the indirection added by getItem and makeItems. I think if we hard-coded the items we're passing in then this code will be become more direct (and thereby more readable, at least to me 😅). It becomes a little more difficult to change, but I think this code will be read much much more often than its read, mostly because these parts of the tests probably won't change for a long time. WDYT?

      it(`should not re-render if any items's watchedItemProps did not change`, () => {
        const items = [(
          <EuiContextMenuItem key="A" data-counter={0}>
            Option A
          </EuiContextMenuItem>
        ), (
          <EuiContextMenuItem key="B" data-counter={1}>
            Option B
          </EuiContextMenuItem>
        )]

        // by not passing `watchedItemProps` no changes to items should cause a re-render
        const component = mount(
          <EuiContextMenuPanel
            items={items}
          />
        );

        expect(component.debug()).toMatchSnapshot();

        component.setProps(
          { items: makeItems() },
          () => {
            expect(component.debug()).toMatchSnapshot();
          }
        );
      });

[cjcenizal]

Same comment applies here and to the other test.

[cjcenizal]

Still LGTM! Just had a question about whether setProps is async.

[cjcenizal]

I forgot to mention this before, you can import takeMountedSnapshot from the test module on line 4, and use it here like this: expect(takeMountedSnapshot(component)).toMatchSnapshot();, which will yield a snapshot of regular HTML:

<div
  class="euiContextMenuPanel"
  tabindex="0"
>
  <div>
    <button
      class="euiContextMenuItem"
      data-counter="0"
      type="button"
    >
      <span
        class="euiContextMenu__itemLayout"
      >
        <span
          class="euiContextMenuItem__text"
        >
          Option A
        </span>
      </span>
    </button>
    <button
      class="euiContextMenuItem"
      data-counter="1"
      type="button"
    >
      <span
        class="euiContextMenu__itemLayout"
      >
        <span
          class="euiContextMenuItem__text"
        >
          Option B
        </span>
      </span>
    </button>
  </div>
</div>

It's definitely not necessary to use this in these tests, but I thought I should mention it!

[cjcenizal]

The tests still execute both assertions if this line is removed. Is this a defensive measure in situations where setProps is asynchronous? I did some digging to see if this was the case but Enzyme's docs don't mention anything about sync vs async, so this is new ground for me.

[cjcenizal]

jenkins test this

[chandlerprall]

jenkins test this