[Lens] Create filters on click with bar, line, area charts#57261
[Lens] Create filters on click with bar, line, area charts#57261mbondyra merged 37 commits intoelastic:masterfrom
Conversation
wylieconlon
added
release_note:enhancement
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
| import { EuiIcon, EuiText, IconType, EuiSpacer } from '@elastic/eui'; | ||
| import { FormattedMessage } from '@kbn/i18n/react'; | ||
| import { i18n } from '@kbn/i18n'; | ||
| import { npStart } from 'ui/new_platform'; |
There was a problem hiding this comment.
Definitely need to remove this
There was a problem hiding this comment.
Side note: I cleaned up the shimmed plugin structure in #56976
| handlers, | ||
| }: XYChartRenderProps & { | ||
| handlers: IInterpreterRenderHandlers; | ||
| }) { |
There was a problem hiding this comment.
@ppisljar I can't tell if it's better to do handlers.event(...) vs npStart.plugins.uiActions.executeTriggerActions('VALUE_CLICK_TRIGGER',, can you clarify the expected use?
There was a problem hiding this comment.
handlers.event is better because it avoids direct, legacy access of npStart.plugins.uiActions.
There was a problem hiding this comment.
@stacey-gammon could you clarify what API on handlers can be used to executeTriggerActions? I am not finding any API that could used to get access to uiActions via handlers?
There was a problem hiding this comment.
I thought the plan was to expose uiActions on handles, either from work Peter was doing, or Wylie. Afaik it is not exposed on handler on master, but I haven't kept up with the status.
| column: table.columns.findIndex(col => col.id === layer.splitAccessor), | ||
| value: pointValue, | ||
| }); | ||
| } |
There was a problem hiding this comment.
@markov00 This logic is the logic I was expecting to have included in the chart library itself
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
# Conflicts: # src/legacy/core_plugins/data/public/actions/value_click_action.ts
mbondyra
force-pushed
the
lens/click-actions
branch
from
231801d to
916f9dd
Compare
…ck-actions # Conflicts: # x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.tsx
mbondyra
force-pushed
the
lens/click-actions
branch
from
6f21737 to
5cd1af8
Compare
wylieconlon
left a comment
wylieconlon
left a comment
There was a problem hiding this comment.
If I could approve my own PR, I would!
| data, | ||
| editorFrame: editorFrameSetupInterface, | ||
| formatFactory, | ||
| executeTriggerActions: npStart.plugins.uiActions.executeTriggerActions, |
There was a problem hiding this comment.
This is basically tech debt already, but I don't see an alternative that exists.
There was a problem hiding this comment.
don't use npStart here, pass your dependencies into setup method.
once you are in new platform the second parameter to setup() wil lbe all the plugin contracts.
currently you already get the expressions, data and embeddable there ... add uiActions (between line 86 and 87) there as well
wylieconlon
changed the title
| const pluginInstance = plugin(); | ||
| pluginInstance.setup(npSetup.core, { | ||
| ...npSetup.plugins, | ||
| uiActions: npStart.plugins.uiActions, |
There was a problem hiding this comment.
This does not look correct. We cannot (technically in legacy platform we can, but it would break as soon as we move to Kibana platform) inject a start dependency into the setup phase of the plugin (that runs before the start phase).
If we get access to the uiActions only via the start contract, we need to pass it down from the start method of our plugin instead. We can use the createGetterSetter utils in kibana_utils to store a reference of it in start and use it later when clicking.
There was a problem hiding this comment.
yup, thats right ... general problem, what to do when you need to register something to a registry (needs to happen in setup phase) but the thing you are registering has a start dependencies.
what we usually do is
- create
services.tsfile and create uiActions getter and setter (check visualizations plugin, data plugin, expression plugin, ...) - wherever you need the uiActions use the getter
- in your plugins start lifecycle method use the setter to set the reference
- make sure you don't expose anything using the getter as a static export
There was a problem hiding this comment.
We have the same problem with “formatFactory” (currently importer via legacy imports, but actually exposed in the start phase. In a separate Pr I solved this problem by passing down a promise of the contract and resolve if needed
There was a problem hiding this comment.
Yea, I've been solving this @flash1293's way, which I think is the correct way, it follows the pattern of coreSetup.getStartServices. So what would get passed down to handlers I think is async getUiActionsService() and that can be something like:
const getUiActionsService: async () => await coreSetup.getStartServices()[1].uiActions;
Expression fns are async so I think this should work fine. I think we should prefer that pattern over the getter/setter.
cc @joshdover
| import { LensMultiTable } from '../types'; | ||
| import React from 'react'; | ||
| import { shallow } from 'enzyme'; | ||
| import { mount, shallow, ReactWrapper } from 'enzyme'; |
There was a problem hiding this comment.
Let's use import { mountWithIntl } from 'test_utils/enzyme_helpers' instead, since that takes care about our i18n framework. Even if we don't have any call in it right now, this will prevent us from needing to change all calls later, in case we would ever introduce any messages within this component.
| const table = data.tables[layerId]; | ||
| const rows = table.rows.filter( | ||
| row => | ||
| !(splitAccessor && !row[splitAccessor] && accessors.every(accessor => !row[accessor])) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| row => layer.xAccessor && row[layer.xAccessor] === xyGeometry.x | ||
| ), | ||
| column: table.columns.findIndex(col => col.id === layer.xAccessor), | ||
| value: (xyGeometry as GeometryValue).x, |
There was a problem hiding this comment.
| value: (xyGeometry as GeometryValue).x, | |
| value: xyGeometry.x, |
We already cast above.
| const xySeries = series as XYChartSeriesIdentifier; | ||
| const xyGeometry = geometry as GeometryValue; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| export interface XYChartSeriesIdentifier { | ||
| yAccessor: string | number; | ||
| splitAccessors: Map<string | number, string | number>; // does the map have a size vs making it optional | ||
| seriesKeys: Array<string | number>; | ||
| } |
There was a problem hiding this comment.
It seems this type is now exposed by the chart library itself and we don't need to put this here manually.
| const timeFieldName = | ||
| xDomain && xAxisFieldName ? { timeFieldName: xAxisFieldName } : null; |
There was a problem hiding this comment.
🎨 Code Style suggestion:
const timeFieldName = xDomain && xAxisFieldName;
// ...
const context = {
timeFieldName, // no weird spread needed.
}Don't see why we would want to wrap it in an object to spread it further down immediately again?
| }); | ||
| } | ||
|
|
||
| const xAxisFieldName = table.columns.find(col => col.id === layer.xAccessor)?.meta |
There was a problem hiding this comment.
Best example of why unknown should always be preferred over any. Since meta is any we suddenly start spreading any here, and everything accessing xAxisFieldName will suddenly become any too.
Let's type here explicitly to prevent that: const xAxisFieldName: string | undefined = ....
| import { EditorFrameSetup } from '../types'; | ||
|
|
||
| import { UiActionsStart } from '../../../../../../src/plugins/ui_actions/public'; | ||
| import { setExecuteTriggerActions } from './services'; |
There was a problem hiding this comment.
🎨 Could we please insert an empty line after the imports.
|
|
||
| editorFrame.registerVisualization(xyVisualization); | ||
| } | ||
| start(core: CoreStart, { uiActions }: XyVisualizationPluginStartPlugins) { |
There was a problem hiding this comment.
🎨 ... and an empty line before new methods in a class
timroes
left a comment
timroes
left a comment
There was a problem hiding this comment.
Left two last code style comments. Besides that this looks good for me. Tested on Chrome Linux, in the editor and on dashboards. Seems to create filters correctly for several different chart configurations (we know there are some weird behaviors around multiple layers with mixed primary/non-primary time fields). LGTM
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
8 participants
Release note
Lens now supports filtering in dashboards and the Lens editor for the bar, line and area visualizations.
Closes #52806
Checklist
Delete any items that are not applicable to this PR.