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

[Lens] Allow visualizations to provide a dimension editor #67560

Merged
merged 11 commits into from
Jun 1, 2020

Conversation

wylieconlon
Copy link
Contributor

@wylieconlon wylieconlon commented May 27, 2020

This sets up an optional function renderDimensionEditor which can be provided by visualizations to add extra configuration to a specific dimension. None of these new editors are added in the PR, but this sets up future work that would let the user change the axis, text alignment, color, or other visualization-specific styles.

Screenshot 2020-05-28 17 06 49

Screenshot 2020-05-28 17 11 24

When there is no editor registered, only one tab is shown:

Screenshot 2020-05-28 17 11 53

Closes #67225

Checklist

Delete any items that are not applicable to this PR.

@wylieconlon wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.9.0 labels May 27, 2020
@wylieconlon wylieconlon requested review from timroes, flash1293, mbondyra and a team May 27, 2020 21:04
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@mbondyra
Copy link
Contributor

@elasticmachine merge upstream

@mbondyra
Copy link
Contributor

@wylieconlon good job! In this PR you're putting vis dimension editor right below the datasource dimension editor, don't we need a design for it for the placement? Another option would be separate tab. I am happy with merging it now and improving later.

mockDatasource = createMockDatasource('ds1');
});

it('should fail to render if the public API is out of date', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this test. Could you explain? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's testing another one of the edge cases in rendering, where the known layers don't match. This tests that when rendering layer first without any information about that ID, it doesn't throw errors.

@wylieconlon
Copy link
Contributor Author

@mbondyra This placement is based on the design in #53663

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested if it doesn't break anything and with adding a very silly implementation of renderDimensionEditor:

  renderDimensionEditor (domElement,props ){
    render(<div>DimensionEditor</div>,
      domElement
    );
  },

@wylieconlon wylieconlon requested a review from cchaos May 28, 2020 19:22
@cchaos
Copy link
Contributor

cchaos commented May 28, 2020

Based on the mock from #53660, the configurations need to be rendered to the right of the aggregations list, not below.

Image 2020-05-28 at 3 49 56 PM

@wylieconlon
Copy link
Contributor Author

@cchaos I don't think that's a realistic option because of the separate ownership of the two areas: The top section is entirely rendered by the datasource, and the bottom section is entirely rendered by the visualization.

@cchaos
Copy link
Contributor

cchaos commented May 28, 2020

Then we will need to move quicker towards the next phase which is using Tabs to separate calculation and style/formatting concerns. The mock below shows two tabs, one where it's simply the aggregation and it's configurations, the other is for coloring and other stylistic options that are not necessarily tied to the aggregation type.

Popover

Actual content subject to change.

cc @AlonaNadler

I'm trying to avoid such a large void where there is no content and as the list of possible aggregations grows, that void will too.

@wylieconlon
Copy link
Contributor Author

@cchaos Tabs are possible, but we aren't going to have very many options at the beginning. Imagine having a "Format & Style" tab with only one setting- is that acceptable?

@cchaos
Copy link
Contributor

cchaos commented May 28, 2020

Yep! It's better for the user than moving UI a bunch.

@wylieconlon wylieconlon requested a review from a team as a code owner May 28, 2020 21:13
@wylieconlon
Copy link
Contributor Author

@cchaos done

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wylieconlon! Now we have a good place to start adding in style customizations.

defaultMessage: 'Format & style',
}),
content: (
<div className="lnsVisualizationDimensionEditor">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit for BEM naming

Suggested change
<div className="lnsVisualizationDimensionEditor">
<div className="lnsLayerPanel__styleEditor">

@flash1293
Copy link
Contributor

If I see this correctly, each visualization can only register a dimension editor for all dimensions at once, so if a visualization only needs this this in certain places (but not in all of them), an empty tab would be rendered, correct?

E.g. let's say we implement fitting function next, this makes only sense for the current x axis dimension, so the dimension editor would be empty for y axis dimensions, but the tab would still be rendered, right?

IMHO we should provide a way to not render a tab as well. Maybe having an optional property

shouldRenderDimensionEditor( props: VisualizationConfigProps<T> & {
  groupId: string;
  accessor: string; }): boolean;

as well.

@flash1293
Copy link
Contributor

Maybe better: Another option would be to provide a list of groups you want the editor to show. That would discourage developers from showing the tab and hiding it based on configuration of the dimension (which would be confusing for end users).

@wylieconlon
Copy link
Contributor Author

@flash1293 It sounds like we already have a solution for this, which is based on the way dimension groups are defined by the visualization. I can add something for this.

@flash1293
Copy link
Contributor

@wylieconlon I see, making this part of VisualizationDimensionGroupConfig seems straightforward and like a good tradeoff

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@wylieconlon wylieconlon merged commit be51ca6 into elastic:master Jun 1, 2020
@wylieconlon wylieconlon deleted the lens/extra-dimension-panel branch June 1, 2020 17:33
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Jun 1, 2020
)

* [Lens] Allow visualizations to provide a dimension editor

* Update to tab style

* Remove table update

* Update class name

* typecheck fix

* Add test

* Require each dimension group to enable editor

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
wylieconlon pushed a commit that referenced this pull request Jun 1, 2020
…67885)

* [Lens] Allow visualizations to provide a dimension editor

* Update to tab style

* Remove table update

* Update class name

* typecheck fix

* Add test

* Require each dimension group to enable editor

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Marta Bondyra <marta.bondyra@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Architectural requirement: Visualizations can extend dimension editors
6 participants