[Lens] Fieldless operations#78080
Conversation
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
| selectedOperationDefinition?.input === 'field' || | ||
| (incompatibleSelectedOperationType && | ||
| operationDefinitionMap[incompatibleSelectedOperationType].input === 'field') ? ( | ||
| <EuiFormRow |
There was a problem hiding this comment.
For reviewers: this is an indentation change, not a functional change inside this code.
| expect(await find.allByCssSelector('.echLegendItem')).to.have.length(3); | ||
| }); | ||
|
|
||
| it('should create an xy visualization with filters aggregation', async () => { |
There was a problem hiding this comment.
In my opinion this function test is covering the simplest case, and I think I've covered the other cases in jest-based tests
cchaos
left a comment
There was a problem hiding this comment.
Screenshots LGTM, no SASS changes 👍
flash1293
left a comment
There was a problem hiding this comment.
Didn't get to a thorough review yet (will do that tomorrow), but I couldn't find any problems with the behavior. The biggest concern I have is to remove the type param from some of the operation functions and the param editor - these casts don't look necessary but maybe I'm missing something.
I started these changes:
interface BaseOperationDefinitionProps<C extends BaseIndexPatternColumn> {
type: C['operationType'];
/**
* The priority of the operation. If multiple operations are possible in
* a given scenario (e.g. the user dragged a field into the workspace),
* the operation with the highest priority is picked.
*/
priority?: number;
/**
* The name of the operation shown to the user (e.g. in the popover editor).
* Should be i18n-ified.
*/
displayName: string;
/**
* This function is called if another column in the same layer changed or got removed.
* Can be used to update references to other columns (e.g. for sorting).
* Based on the current column and the other updated columns, this function has to
* return an updated column. If not implemented, the `id` function is used instead.
*/
onOtherColumnChanged?: (
currentColumn: C,
columns: Partial<Record<string, IndexPatternColumn>>
) => C;
/**
* React component for operation specific settings shown in the popover editor
*/
paramEditor?: React.ComponentType<ParamEditorProps<C>>;
/**
* Function turning a column into an agg config passed to the `esaggs` function
* together with the agg configs returned from other columns.
*/
toEsAggsConfig: (column: C, columnId: string, indexPattern: IndexPattern) => unknown;
/**and could remove the casts without running into type errors.
| }, | ||
| isTransferable: (column, newIndexPattern) => { | ||
| const newField = newIndexPattern.fields.find((field) => field.name === column.sourceField); | ||
| const c = column as DateHistogramIndexPatternColumn; |
There was a problem hiding this comment.
It seems a bit ugly we need those casts everywhere. What was the reason for typing column by the column type of the current operation?
|
@flash1293 We definitely needed type casts in a few places, but I agree that it was not great to push them into each operation definition. I think I have a solution to push the casting into a shared place instead of each child. |
|
@wylieconlon Sounds great, thanks. I will give it a thorough look tomorrow morning. |
|
@elasticmachine merge upstream |
There was a problem hiding this comment.
Tested in Chrome and Firefox and didn't notice any issues, LGTM.
I think the type setup is fine that way, I played around a bit but didn't find a better solution.
The only thing I noticed is a small inconsistency in the wording of the error message on switching from filters to something else:

There is no field selected so it's not really a "different field", it's just a field
|
@flash1293 Thanks for the suggestion, I've added a new warning which was "To use this function, select a field." to handle this case. |
|
@elasticmachine merge upstream |
mbondyra
left a comment
There was a problem hiding this comment.
Tested on Safari, all works as expected. Code LGTM.
💚 Build SucceededMetrics [docs]page load bundle size
History
To update your PR or re-run it, just comment with: |
* [Lens] Fieldless operations * Overhaul types * Fix invalid state and add tests * Fix types * Small cleanup * Add additional error message * Reset field selector to empty state when invalid Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: Fix APM lodash imports (elastic#78438) Add deprecated message to tile_map and region_map visualizations. (elastic#77683) Fix Lens smokescreen flaky tests (elastic#78566) updated discover with alt text (elastic#77660) Fix types (elastic#78619) Update tutorial-visualizing.asciidoc (elastic#76977) Update tutorial-discovering.asciidoc (elastic#76976) [Search] Error notification alignment (elastic#77788) Update tutorial-define-index.asciidoc (elastic#76975) [Lens] Fieldless operations (elastic#78080) [Usage Collection] [schema] Explicit "array" definition (elastic#78141) Update tutorial-define-index.asciidoc (elastic#76973) Fix --no-basepath references in doc (elastic#78570) Move StubIndexPattern to data plugin and convert to TS. (elastic#78518) Index pattern class - remove unused methods (elastic#78538) [Security Solution] [ALL] Eliminates all console.error and console.warn from Jest output (elastic#78523) [Actions] avoids setting a default dedupKey on PagerDuty (elastic#77773) First stab at developer-focussed saved objects docs (elastic#71430) remove unnecessary config validations (elastic#78527)
This PR begins categorizing operations in Lens by the type of input that they take, switching from the previous state of field-based operations to having two types. In the future, we will add a third type to support the calculations internal API.
Screenshots
Technical summary of changes
There are two types of changes here: changes that affect the UI for building dimensions, and then a bunch of Typescript changes associated.
Changes that affect the UI:
There are very few changes here:
input: 'none'orinput: 'field', which is read by the dimension editor.input: 'none'Typescript changes:
In addition to the required
inputproperty, we now use a discriminated union type on the operation definitions. This lets us define use-case-specific functions for each operation input, while maintaining some shared logic on all the operations.Closes #75231
Checklist