Skip to content

[Lens] Split up dimension panel code#80423

Merged
wylieconlon merged 9 commits intoelastic:masterfrom
wylieconlon:lens/refactor-dimensions
Oct 19, 2020
Merged

[Lens] Split up dimension panel code#80423
wylieconlon merged 9 commits intoelastic:masterfrom
wylieconlon:lens/refactor-dimensions

Conversation

@wylieconlon
Copy link
Copy Markdown
Contributor

@wylieconlon wylieconlon commented Oct 13, 2020

There are three main changes in this PR, all refactorings that I found important as part of creating a reference-based editor for operations:

  1. Pass around the entire layer object instead of columns. This lets us use different parts of the layer in the future.
  2. Split up the dimension_panel into three files:
    a. dimension_panel keeps only the UI components, and only UI tests
    b. droppable deals with only dragging and dropping logic, including what happens on drop
    c. operation_support is the function which is used to show functions + fields in the panels. Splitting it into a separate file so that it can be mocked.

Checklist

@wylieconlon wylieconlon added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.11.0 labels Oct 13, 2020
@wylieconlon wylieconlon requested review from a team, dej611, flash1293 and mbondyra October 13, 2020 22:12
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 13, 2020

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

dej611
dej611 approved these changes Oct 15, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

The code looks ok 👍 . Useful mergeLayer function, we should normalize the whole lens codebase with it!
I've left only some minor comments here and there.

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.ts Outdated

// Time to replace
props.setState({
...props.state,
Copy link
Copy Markdown
Contributor

@dej611 dej611 Oct 15, 2020

Choose a reason for hiding this comment

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

nit: can you use mergeLayer here?

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/operation_support.ts Outdated
Comment on lines +41 to +51
if (supportedOperationsByField[operation.field]) {
supportedOperationsByField[operation.field]!.push(operation.operationType);
} else {
supportedOperationsByField[operation.field] = [operation.operationType];
}

if (supportedFieldsByOperation[operation.operationType]) {
supportedFieldsByOperation[operation.operationType]!.push(operation.field);
} else {
supportedFieldsByOperation[operation.operationType] = [operation.field];
}
Copy link
Copy Markdown
Contributor

@dej611 dej611 Oct 15, 2020

Choose a reason for hiding this comment

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

nitpick: I always find a bit harder to read these conditionals that handles add or create in different ways. What about check the existence of the entry and just push?

supportedOperationsByField[operation.field] = supportedOperationsByField[operation.field] || [];
supportedOperationsByField[operation.field].push(operation.operationType);

supportedFieldsByOperation[operation.operationType] = supportedFieldsByOperation[operation.operationType] || [];
supportedFieldsByOperation[operation.operationType].push(operation.field);

Copy link
Copy Markdown
Contributor Author

@wylieconlon wylieconlon Oct 16, 2020

Choose a reason for hiding this comment

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

I think I understand your point, but I'm not sure your proposal is more clear. I'm going to try using the spread operator, but maybe this is just as bad?

        supportedFieldsByOperation[operation.operationType] = [
          ...(supportedFieldsByOperation[operation.operationType] ?? []),
          operation.field,
        ];

x-pack/plugins/lens/public/indexpattern_datasource/state_helpers.ts
updatedColumn.label = oldColumn.label;
}

const layer = {
Copy link
Copy Markdown
Contributor

@dej611 dej611 Oct 15, 2020

Choose a reason for hiding this comment

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

This is used below with the spread operator: maybe it can be saved here?

const layer = state.layers[layerId];

flash1293
flash1293 approved these changes Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, tested in Chrome and didn't notice any regressions.

@wylieconlon
Copy link
Copy Markdown
Contributor Author

wylieconlon commented Oct 16, 2020

@elasticmachine merge upstream

@wylieconlon
Copy link
Copy Markdown
Contributor Author

wylieconlon commented Oct 16, 2020

@elasticmachine merge upstream

@mbondyra
Copy link
Copy Markdown
Contributor

mbondyra commented Oct 19, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Oct 19, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
lens 564 566 +2

async chunks size

id before after diff
lens 1.0MB 1.0MB +928.0B

History

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

@mbondyra mbondyra mentioned this pull request Oct 19, 2020
Merged
8 tasks
@wylieconlon wylieconlon merged commit f0023ed into elastic:master Oct 19, 2020
@wylieconlon wylieconlon deleted the lens/refactor-dimensions branch October 19, 2020 18:02
@wylieconlon wylieconlon mentioned this pull request Oct 19, 2020
Merged
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Oct 19, 2020
@kibanamachine @wylieconlon
[Lens] Split up dimension panel code (elastic#80423)
06c8b42 
* [Lens] Split up dimension panel code

* Fix test failures

* Style updates

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
wylieconlon pushed a commit that referenced this pull request Oct 19, 2020
@kibanamachine
[Lens] Split up dimension panel code (#80423) (#81030)
066fa9c 
* [Lens] Split up dimension panel code

* Fix test failures

* Style updates

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dej611 dej611 dej611 approved these changes

@flash1293 flash1293 flash1293 approved these changes

@mbondyra mbondyra mbondyra approved these changes

Assignees

No one assigned

Labels

Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.11.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wylieconlon @elasticmachine @mbondyra @kibanamachine @dej611 @flash1293