Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createAggConfig should allow order-specific additions #3159

Closed
joshuarrrr opened this issue Dec 29, 2022 · 0 comments · Fixed by #3160
Closed

createAggConfig should allow order-specific additions #3159

joshuarrrr opened this issue Dec 29, 2022 · 0 comments · Fixed by #3160
Assignees
Labels
enhancement New feature or request unified visualization UX v2.5.0 'Issues and PRs related to version v2.5.0' vis builder visualizations Issues and PRs related to visualizations

Comments

@joshuarrrr
Copy link
Member

joshuarrrr commented Dec 29, 2022

Is your feature request related to a problem? Please describe.

When creating a new AggConfig, the default behavior is to also add the newly created AggConfig to the existing list of aggConfigs:

However, order matters when specifying aggregations, and the aggregation Schema supports the property mustBeFirst, which is used by the pie chart to ensure that slices are calculated accurately. The simplest, and likely best, fix is to update createAggConfig to take an additional parameter that allows the new AggConfig to be added to the beginning of the aggConfigs array rather than the end.

Describe the solution you'd like

Maintain existing behavior of createAggConfig, but add a new parameter that can switch the insertion point from the end of the array to the beginning.

Describe alternatives you've considered

The existing logic to enforce mustBeFirst constraints is in the vis_default_editor plugin. But for adding pie charts to VisBuilder, we'd have to recreate this logic, since VisBuilder doesn't use the vis_default_editor. In addition, we'd just like to always order the aggs correctly, rather than displaying and error message and forcing the user to reorder (as the current Pie chart visualization does)

Doing so downstream of the agg service means separating the creation of the new AggConfig from the process of adding it to the existing aggConfigs, and ends up with lots of duplication of the order checking logic and direct manipulation of the aggConfigs array. For example, one implementation might look something like this:

       const aggConfig = aggConfigs?.createAggConfig(
        {
          type: allowedAggTypes[0] || validAggTypes[0],
          schema: schema.name,
          params: {
            field: fieldName,
          },
        },
        {
          addToAggConfigs: false,
        }
      );

      const newAggs = schema.mustBeFirst ? [aggConfig, ...aggs] : [...aggs, aggConfig];
      const newAggConfigs = aggService.createAggConfigs(indexPattern, cloneDeep(newAggs));

Conceptually, the agg service should provide the right methods and tooling to add, remove, and reorder agg configs without requiring lots of manual array manipulation and cloning.

Additional context

Blocks #1619

@joshuarrrr joshuarrrr added enhancement New feature or request vis builder visualizations Issues and PRs related to visualizations unified visualization UX labels Dec 29, 2022
@joshuarrrr joshuarrrr added the v2.5.0 'Issues and PRs related to version v2.5.0' label Dec 29, 2022
@joshuarrrr joshuarrrr self-assigned this Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request unified visualization UX v2.5.0 'Issues and PRs related to version v2.5.0' vis builder visualizations Issues and PRs related to visualizations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant