-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Embeddables Rebuild] Data table example #181768
[Embeddables Rebuild] Data table example #181768
Conversation
/ci |
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
true | ||
); | ||
}, | ||
getDisplayName: () => |
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.
lets remove i18n translations from examples.
- Examples target developers and all development is done in English
- Creating translations cost money
- Translations bloat bundle size
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 like keeping the i18n
in examples because I'd like it to show our best practices and I'd like the code to look / work as close as possible to the real world code.
I agree that all development on Kibana is done in English, but I'm pretty sure usages in examples
don't get translated - as they shouldn't:
There are 0 instances of embeddableExamples
in the translation files, and some i18n
usages have been around for over 4 years. embeddableExamples.searchableListContainer.displayName
and embeddableExamples.searchableListContainer.displayName
are roughly that old.
As for the bundle sizes, this only applies when user is running Kibana with --run-examples
- which only developers should be doing.
import { initializeDataTableQueries } from './data_table_queries'; | ||
import { DataTableApi, DataTableProvider, DataTableSerializedState } from './types'; | ||
|
||
export const isDataTableProvider = (api: unknown): api is DataTableProvider => { |
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.
Looks like isDataTableProvider
is not used any where and can be removed
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.
Good call, removed in e19133b
<> | ||
<EuiScreenReaderOnly> | ||
<span id="dataTableReactEmbeddableAria"> | ||
{i18n.translate('embeddableExamples.dataTable.ariaLabel', { |
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.
remove i18n
]); | ||
if (!defaultDataView) { | ||
throw new Error( | ||
i18n.translate('embeddableExamples.dataTable.noDataViewError', { |
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.
remove i18n
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.
Also, this message does not align with if statement check for !defaultDataView
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.
Updated message in e19133b
|
||
// run query whenever the unified search state changes | ||
const runQuery = async (unifiedSearchState: DataTableQueryState) => { | ||
const { filters, query, timeRange, dataView } = unifiedSearchState; |
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.
Should also consume timeslice so example response to timeslider control
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.
Added time slice in e19133b
const { rawResponse: resp } = await lastValueFrom( | ||
searchSource.fetch$({ | ||
abortSignal: abortController.signal, | ||
sessionId: v4(), // todo, search sessions |
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.
Use search session from fetch$
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.
Good catch! Fixed in e19133b
|
||
export type DataTableApi = DefaultEmbeddableApi<DataTableSerializedState> & PublishesDataLoading; | ||
|
||
export type DataTableProvider = PublishesDataViews & PublishesSelectedFields; |
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.
DataTableProvider type can be removed. Its not used any where
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.
Ah - missed this. Removed in 2b6cf98
I went back and forth on fixing this or not fixing this back when I implemented the field list. Clearing the fields on data view change felt like we were wiping user-selections as part of an unrelated action - but it looks like Discover clears the fields when the data view is changed, so these should probably do the same. At the end of the day though it is just an example, so we really shouldn't be trying to make the UX perfect. I think I can update it to do that really quickly though... Edit - changed this behaviour in f4dcafd |
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.
LGTM - nice example to show how sibling embeddables can interact
code review, tested in chrome
#181761 is merged now. Would you mind adding |
On it! |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Public APIs missing exports
History
To update your PR or re-run it, just comment with: |
Adds a new data table example that shows how embeddables can interact with their siblings
Summary
Closes #176180
Communication with siblings
This PR uses the method
listenForCompatibleApi
, to listen toThis works alongside the field list Embeddable example to create the behaviour above.