Provide tools to search bar as nodes

[cjcenizal]

Let's compare and contrast this with the config-object-based approach of #452:

    const toolbar = {
      leftTools: [
        {
          type: 'button',
          name: 'Delete Items',
          icon: 'trash',
          color: 'danger',
          available: (table) => table.selection && table.selection.length > 0,
          onClick: (table) => {
            store.deleteUsers(...table.selection.map(user => user.id));
            table.refresh();
          }
        }
      ],
      search: {
        box: {
          incremental: this.state.incremental
        },
        filters: !this.state.filters ? undefined : [
          {
            type: 'is',
            field: 'online',
            name: 'Online',
            negatedName: 'Offline'
          },
          {
            type: 'field_value_selection',
            field: 'nationality',
            name: 'Nationality',
            multiSelect: false,
            options: store.countries.map(country => ({
              value: country.code,
              name: country.name,
              view: `${country.flag} ${country.name}`
            }))
          }
        ]
      }
    };

[nreese]

I like the flexibility that passing in actions as nodes provides. This gives the table consumer a lot of control and future proofs the component for future use cases.

[nreese]

2 returns in method. Second return that renders node never gets used

[uboness]

Can you paste an example snippet in the description (so you can compare)?

Flexibility comes in different shapes any forms... But I'll wait for the snippet

[chrisronline]

I'll let @cjcenizal clarify exactly, but I think the discussion is around:

{
  type: 'button',
  name: 'Delete Items',
  icon: 'trash',
  color: 'danger',
  available: (table) => table.selection && table.selection.length > 0,
  onClick: (table) => {
    store.deleteUsers(...table.selection.map(user => user.id));
    table.refresh();
  }
}

is less expressive than:

table.selection && table.selection.length > 0
? <Button
    iconType="trash"
    color="danger"
    onClick={(table) => {
      store.deleteUsers(...table.selection.map(user => user.id));
      table.refresh();
    }}
  >
    Delete Items
  </Button>
: null

With the general spirit being that we can create expressive UIs through React componentry and that expressiveness leads to better clarity, rather than potentially needing to infer meaning based on config property names (obviously, React component props can suffer from this too, so it's not perfect).

Like in this example, I actually wasn't clear on what available meant but seeing it in the form I provided, for me at least, makes it much more clear. I don't know if we can/should create a steadfast rule for this, but this example feels like a good candidate for a more expressive option.

[cjcenizal]

Thanks @chrisronline, I couldn't have put it nearly as well as you just did.

[nreese]

Is this still an experiment or can it be merged?

[cjcenizal]

@nreese I'd like to update a couple other examples and add some tests and I then I think it will be ready for a final review & merge.

[cjcenizal]

@bmcconaghy @jen-huang @chrisronline @nreese Well, turns out I just needed to add a test. This is ready for review. @uboness mentioned lingering concerns about the necessary boilerplate. The only boilerplate I can see that we need is the check against selection to see if we need to hide or show tools based on whether any rows have been selected. Can anyone spot anything we might be missing here?

[nreese]

lgtm

[chrisronline]

LGTM

[uboness]

@cjcenizal where is the comparison your referring to? am I missing something? (we said we're going to have a comprehensive example and compare).

[uboness]

ah... I see it now... yea... that's a boilerplate I'd rather avoid... I'll take this:

leftTools: [
        {
          type: 'button',
          name: 'Delete Items',
          icon: 'trash',
          color: 'danger',
          available: (table) => table.selection && table.selection.length > 0,
          onClick: (table) => {
            store.deleteUsers(...table.selection.map(user => user.id));
            table.refresh();
          }
        }
      ],

over this:

renderToolsLeft() {
    const selection = this.state.selection;

    if (selection.length === 0) {
      return;
    }

    const onClick = () => {
      store.deleteUsers(...selection.map(user => user.id));
      this.setState({ selection: [] });
    };

    return (
      <EuiButton
        color="danger"
        iconType="trash"
        onClick={onClick}
      >
        Delete {selection.length} Users
      </EuiButton>
    );
  }

any day of the week. And... you now need to track selection state yet again... more boilerplate...

but... seems like kibana likes to boilerplate... 🤷‍♂️... do whatever you think is right

[cjcenizal]

Thanks @uboness. We're going to try the approach of passing in nodes first. If the boilerplate proves to be too burdensome then it will be good that we have your config-based approach to fall back to.