Refactor pagination and selection to emphasize primitive values, both as state and props.

[cjcenizal]

@stacey-gammon Based on elastic#10434, I wanted to try out a couple ideas and see what you and @weltenwort thought.

Instead of assigning instances of Pager and SelectedIds to the state object, I tried to reserve that object for primitive values only. In this case, currentPage (number) and selectedIds (array).

Then I tried to create pure functional helpers for reasoning and manipulating this state. I created a new Pager class (which needs to be extracted) and refactored ItemSelectionActions to export pure functions to support selection behavior. I was also able to remove the dependency on PagerActions.

The ideas I want to focus on are:

• Emphasizing primitives makes code easier to follow. If our data consists of primitive values, then it's easier to create pure functions for operating on them. They're explicit, so the code communicates its meaning without requiring you to learn an abstraction.
• Pure functions are easier to understand. I don't to think about side effects or mutated state. When invoking a pure function, all I have to think about is what goes in and what comes out.
• I think passing props multiple times down the component tree is a code smell. I'd like to hear what @kjbekkelund thinks about this and what kinds of solutions they used on the Cloud team. Personally, I think that this is a sign that we should be using composition more and passing props less.

Edit: I just did a second stage of exploration which builds on this one (cjcenizal#16)

[cjcenizal]

I made this 2 items per page for demo purposes.

[cjcenizal]

The duplication of the props here and for LandingPageToolBar make me question this interface. Maybe we shouldn't encapsulate the composition of this UI. What if we create a function here called getPagerButtons() which will render the pagination buttons? We could call it once and then provide the buttons to both LandingPageToolBar and LandingPageToolBarFooter as children, or possibly as a prop.

[weltenwort]

Yes, maybe the LandingPageToolBar encapsulation is unnecessary. Just composing KuiSectionedToolBar, KuiToolBarSearchBox and KuiToolBarPager might be enough. Roughly:

  render() {
    const { filter, pager, selectedIds } = this.state;
    const pager = (
      <KuiToolBarPager
        startNumber={0}
        endNumber={0}
        totalItems={10}
        hasPreviousPage={this.hasPreviousPage}
        hasNextPage={this.hasNextPage}
      />
    );

    return (
      <div>
        <KuiSectionedToolBar>
          <KuiToolBarSearchBox filter={filter} onFilter={onFilter}/>
          { actionButtons }
          { pager }
        </KuiSectionedToolBar>
        { this.getTableContents() }
        <div className="kuiToolBarFooter">
          <KuiSelectedItemsFooterSection selectedItemsCount={selectedIds.length} />
          { pager }
        </div>
      </div>
    );
  }

[cjcenizal]

We're passing so many props down to the component tree, I wonder if there's some refactoring that needs to be done here. This would be something I'd probably want to tackle in a subsequent iteration.

[cjcenizal]

This needs to be extracted.

[weltenwort]

I like what the state looks like, but the actions are still awkwardly scattered. See my comment at elastic#10434 (comment).

[weltenwort]

Yes, maybe the LandingPageToolBar encapsulation is unnecessary. Just composing KuiSectionedToolBar, KuiToolBarSearchBox and KuiToolBarPager might be enough. Roughly:

  render() {
    const { filter, pager, selectedIds } = this.state;
    const pager = (
      <KuiToolBarPager
        startNumber={0}
        endNumber={0}
        totalItems={10}
        hasPreviousPage={this.hasPreviousPage}
        hasNextPage={this.hasNextPage}
      />
    );

    return (
      <div>
        <KuiSectionedToolBar>
          <KuiToolBarSearchBox filter={filter} onFilter={onFilter}/>
          { actionButtons }
          { pager }
        </KuiSectionedToolBar>
        { this.getTableContents() }
        <div className="kuiToolBarFooter">
          <KuiSelectedItemsFooterSection selectedItemsCount={selectedIds.length} />
          { pager }
        </div>
      </div>
    );
  }

[weltenwort]

Why pass a function instead of the value here?

[cjcenizal]

Good point! I don't think it needs to be.

[weltenwort]

I smell a selector! 🎉 and it smells good 😉

[stacey-gammon]

I think we should be more explicit about whether currentPage is an index or not. I'm working on merging your PR into mine and ran into a bug here because pagesCount can be 0. I'm going to add a new getLastPageIndex function and put the - 1 logic in there. Sound okay?

[stacey-gammon]

Or alternatively, have currentPage be 1 based, not 0 based. Any opinion?

[cjcenizal]

This is a really good point. We should disambiguate whether this number is 0-indexed or 1-indexed in the various places it's referenced. The problem isn't formed clearly enough in my head for me to form an opinion. We can zoom about it if you want, or you can just take a stab at it and I can take a look in code review.

[stacey-gammon]

Another question is how the pager should look if there are no items.
Will it show 0 - 0 of 0?
And if there is 1 item, is it 1 - 1 of 1?
And for 2, 1 - 2 of 2

[cjcenizal]

Let's use @uboness 's idea in elastic#10555

[cjcenizal]

These ideas have been incorporated in a separate PR into the original branch.