-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 user to drag and select a subset of the timeline in the chart (aka brush interaction) #62636
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested this yet, but I left a few comments about the structure of the code.
} | ||
|
||
const table = data.tables[layers[0].layerId]; | ||
const xAxisFieldName: string | undefined = xAxisColumn?.meta?.aggConfigParams?.field; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By accessing the aggConfigParams
property of the table, you are breaking one of the rules that we decided to stick with in Lens: visualizations aren't allowed to write code that is specific to a single datasource. The way we usually would write this code as a result is that instead of checking for aggConfigParams
or the date_histogram
function, you would check for xScaleType === 'time'
or dataType === 'date'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wylieconlon but this is the information we need to pass timeFieldName, so the filter will be applied to time picker and not as a regular filter. I don't see any other way to get it. We do the same thing onClick. Do you think we should extend configuration and add xAxisFieldName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as discussed in the meeting this should be fine as we will be moving field information off the aggConfigParams directly to meta and is something that can be provided by most datasources.
// TODO: simplify the context structure: https://github.com/elastic/kibana/issues/62936 | ||
const context: EmbeddableVisTriggerContext = { | ||
data: { | ||
range: [moment(min).toISOString(), moment(max).toISOString()], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to set the timezone explicitly here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually checked for different timezones and it works correctly right now. How would the potential timezone setting look like?
x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.tsx
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.tsx
Outdated
Show resolved
Hide resolved
}, | ||
}, | ||
formatHint: { id: 'date', params: { pattern: 'HH:mm' } }, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left this comment in a few places, but it will hopefully simplify this test too: we should not use the table metadata, because it's specific to only one data source.
x-pack/legacy/plugins/lens/public/xy_visualization/xy_expression.test.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
@mbondyra I just pushed an update to this PR because I had already made some tweaks locally, and couldn't find a way to submit a PR to your fork. Here is the reasoning behind what I've changed:
The new behavior that I've pushed up has each datasource pass the GIF of new behavior: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavior now LGTM with the tweaks I made. Tested in scenarios including multiple layers and non-primary time fields. The best dataset for this is actually metricbeat
run locally, because it can track the start time of processes.
…-on-lens # Conflicts: # x-pack/legacy/plugins/lens/public/services.ts # x-pack/legacy/plugins/lens/public/xy_visualization/services.ts # x-pack/plugins/lens/public/xy_visualization/index.ts # x-pack/plugins/lens/public/xy_visualization/services.ts # x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior does something that we don't currently support, which is that the time picker is always updated for time fields. I'm approving with the understanding that we will bind all time fields to the time picker in Lens in the near future, which means this behavior is ready for that.
|
||
return [VIS_EVENT_TO_TRIGGER.filter, VIS_EVENT_TO_TRIGGER.brush]; | ||
// case 'lnsDatatable': | ||
// return [VIS_EVENT_TO_TRIGGER.filter]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented code?
removing commented code
@elasticmachine merge upstream |
…-on-lens # Conflicts: # src/plugins/embeddable/public/lib/triggers/triggers.ts # x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
…-on-lens # Conflicts: # x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…chart (aka brush interaction) (elastic#62636) * feat: brushing basic example for time histogram * test: added * refactor: simplify the structure * refactor: move to inline function * refactor * refactor * Always use time field from index pattern * types * use the meta.aggConfigParams for timefieldName * fix: test snapshot update * Update embeddable.tsx removing commented code * fix: moment remov * fix: corrections for adapting to timepicker on every timefield * fix: fix single bar condition * types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>
…chart (aka brush interaction) (#62636) (#64992) * feat: brushing basic example for time histogram * test: added * refactor: simplify the structure * refactor: move to inline function * refactor * refactor * Always use time field from index pattern * types * use the meta.aggConfigParams for timefieldName * fix: test snapshot update * Update embeddable.tsx removing commented code * fix: moment remov * fix: corrections for adapting to timepicker on every timefield * fix: fix single bar condition * types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>
Summary
Closes #58513
Checklist
Delete any items that are not applicable to this PR.
For maintainers