[Lens] Add xy split series support

[chrisdavies]

Add basic split series support to the xy chart in Lens.

Closes #38835

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[flash1293]

This PR gives access to the datasource public API which I think we should not do (at least not like that).

I introduced something similar with #39576 but opened another PR #39772 to remove this, because it opens a can of worms in multiple ways:

• This implementation is buggy, because there isn't only a single datasource api per suggestion request, but one datasource api per table (because each table is backed by a different datasource state). If visualization authors would start to call apis for column meta data, they wouldn't get the meta data for the given table in the suggestion.
• The datasource api is attached to the global state and can dispatch changes - we definitely don't want to do this as part of suggestion building, because each suggestion could change the datasource state to somewhere else (without the suggestion even picked)
• The point above can be fixed by implementing a suggestion-level state handling for the datasource (e.g. while building the suggestion, the datasource can change its state), but as this complicates everything, I don't think we should allow that. We shouldn't move away from the simple model of "datasource suggests table, visualization can pick table and use it to visualize something" IMHO - no further state changes while piecing together a suggestion. Benefits are easier testing and control flow
• We don't need this for querying the datasource for table meta information, because this is already part of the suggestion object (as solved in [Lens] No datasource api in suggestions #39772 )
• We might need this to create dummy column ids (as in this PR), but I have a suggestion how we could avoid this (see below)

Alternative:

The datasource API is only required in the suggestions because it allows the user to generate new column ids. This is currently a stateless operation and IMHO should stay one - generating them should not update the datasource in any way. Currently generating column ids is the responsibility of the data source, but we could shift it to the editor frame. As the generation is completely stateless ATM it could also be a static import, but if we want to make it stateful in the future (e.g. go col1, col2, col3) or something, the editor frame could also handle this because it has access to the datasource and can look at the current table spec. In this case we would pass a function generateColumnId for that down to getSuggestions. But I would suggest we start out with just providing a static helper method for that in the editor frame.

The other way I see to solve this is to have a separate public API on the datasource which is explicitly not allowed to set state in any way, but I'm not sure whether there will be use cases for that in the future.

What do you think, @chrisdavies @wylieconlon

[chrisdavies]

I modified this to no longer use datasources for id generation. But mocking it at every turn seems hacky. Should we inject the id generator into our components / functions, rather than importing them?

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[flash1293]

LGTM aside from the remove button + tests.

It probably makes sense to use a FlexGroup here to show the dimension panel and the remove button side by side (could also be added to the y axis dimension panels, they have the same problem). But we can also tackle this while styling the config panel as a whole, not super important.

[flash1293]

There should be a way to remove existing split column like for y columns.

[chrisdavies]

You can:

That's the split series list.

[flash1293]

@chrisdavies That just deletes the column in the data source, not the accessor in the visualization. If you press the plus button ten times, you have ten columns "to configure" and now way to remove those placeholders.

y axis has separate remove buttons but they are not styled nicely

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[wylieconlon]

I second Joe's comments about deleting the entire column from the split series, not just clearing the contents of the column. I think there is already some code in the XY config panel that does the same for the Y axes, so maybe you can reuse it? I think this is worth doing in another update to this PR so I'm going to request changes.

Also, I've left a suggestion on changing the mocking strategy to be more direct. I think this is a more stylistic issue, but I know you were looking for feedback on this. I don't think that's a blocker though.

In terms of UX feedback which I don't think you need to address here, but I would like to see:

• There are a lot of buttons and forms shown by default in the XY config panel, and I think it's hard to see which one to start with. The default Visualize app has recently made this even simpler by having a single + button which offers the option to "Add split series" or "Add Y axis" and I think we should do the same
• We don't yet support stacking, but split bar charts should default to being stacked. Line and area should not as they are more often used for comparison.
• This currently allows splitting by dates, as in date histograms, and I find that this is not particularly useful because the split labels are not human readable. Is it worth it to only allows splitting by bucketed strings?

[wylieconlon]

Can you take advantage of jest's ability to replace modules with manual mocks? https://jestjs.io/docs/en/manual-mocks

Here's what I was thinking:

In id_generator/__mocks__/index.ts:

export function generateId() {
  return jest.fn();
}

In your test code:

import { generateId } from '../id_generator';

jest.mock('../id_generator');

  test('allows adding y dimensions', () => {
    (generateId as jest.Mock).mockReturnValueOnce('zed');

[chrisdavies]

That looks much nicer. I'll give it a shot.

Joe's on vacation, and I addressed his concerns (methinks).

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[wylieconlon]

Overall the functionality LGTM, thanks for making the test changes! I've left some comments based on what we talked about which I would prefer getting into this PR, but I don't think I need to review again.

[wylieconlon]

Like we talked about- this should create a new column if you remove the last one. Not sure if that logic belongs in this component or the caller, but it looks like given accessors as a prop it belongs in the caller.

[wylieconlon]

I think it's a better experience to remove dates as filter options, so op => op.isBucketed && op.dataType !== 'date'

[chrisdavies]

Agreed, though eventually, it makes sense to split on dates (e.g. split by month or weekday or something of that sort).

[wylieconlon]

Based on our discussion earlier, it seems like this is the best place to write the conditional logic for adding a new column if the last column was removed

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request