[SharedUX] Analytics No Data Page#130974
Conversation
|
@elasticmachine merge upstream |
majagrubic
added
v8.3.0
Team:SharedUX
|
I'm confused, why do we have a solution-specific component (Analytics) in Shared UX? I understand that Analytics doesn't really have a single folder/plugin to pull from, but I would venture we should make one specifically for Analaytics, not add into shared_ux. |
|
But where else would the component live? As you said, there is no Analytics-specific plugin, and since the page is meant to be reused by multiple plugins, it feels |
cchaos
left a comment
cchaos
left a comment
There was a problem hiding this comment.
👍 The design of the page LGTM. I can't speak to the eng of all the new services.
| } | ||
| ); | ||
|
|
||
| export const AnalyticsNoDataPageComponent = ({ kibanaGuideDocLink, onDataViewCreated }: Props) => { |
There was a problem hiding this comment.
Do we really need to create a dedicated component for the Analytics or could we expose different config object and then the solutions use the generic <KibanaNoDataPage /> component with those configs?
It seems that by only exposing the recommended config object it would still leave the possibility to customize some config if needed (e.g. the logo)
It also seems less verbose if we start to have multiple of those. WDYT?
There was a problem hiding this comment.
Clint and I talked about having a config, that could be defined by each plugin the start time. We'd have almost like a register of configs, so we could do something like
const register = useRegister();
const config = register.get(pluginName);
I think this is the next phase of the project. But for now, let's keep it simple. This will solve the problem at hand. What you're saying doesn't really clash with what we have; plugins can still use the generic <KibanaNoDataPage> component, which takes config as a prop.
There was a problem hiding this comment.
Ok that could work too.
that could be defined by each plugin
I thought that "Analytics" encompasses multiple plugins so each individual plugin didn't have to provide the config (in which case they could directly pass it to the generic KibanaNoDataPage.
Just trying to see how we can lower the number of components/tests/stories we need to maintain. If you are on it with Clint all good 👍
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
miscellaneous assets size
History
To update your PR or re-run it, just comment with: |
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { withSuspense } from '@kbn/shared-ux-utility'; |
There was a problem hiding this comment.
We shouldn't be exporting this. Consumers should import directly from the package.
|
|
||
| import { servicesFactory } from './services'; | ||
|
|
||
| let services: SharedUxServices; |
There was a problem hiding this comment.
This is an anti-pattern. We should not be keeping a module-level instance. Can you tell me why this is necessary?
|
|
||
| public stop() {} | ||
| } | ||
| export const getSharedUXServices = () => services; |
There was a problem hiding this comment.
Yeah, we can not do this. This is exposing a stateful variable subject to a start contract in a module-level, stateless variable.
| const services = getSharedUXServices(); | ||
| const { kibanaGuideDocLink } = services.docLinks; | ||
| return ( | ||
| <SharedUxServicesProvider {...services}> |
There was a problem hiding this comment.
Consuming plugins need to do this. Let me know if you need help working around this limitation, I can help.
There was a problem hiding this comment.
This is what I was trying to avoid. If each plugin needs to provide their own service contract and this can't be a
"plug-n-play" component, then I agree that this component is not much needed, as all it does is provide a pre-initialized config.
|
Closing this as this will be moved to a package, and the current implementation is not mergeable. |
6 participants
Summary
This PR adds an Analytics-specific implementation of
KibanaNoDataPage.The inner component has no service dependencies, for easier testing and storybooking; it's not meant to be used by plugins directly.
The
AnalyticsNoDataPagehas service dependencies and can be used by plugins directly, by importing fromshared-uxplugins.Checklist
Delete any items that are not applicable to this PR.
- [ ] Documentation was added for features that require explanation or tutorials- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker listFor maintainers