-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[infra UI] Add overview tab with the first section to asset details view #160924
[infra UI] Add overview tab with the first section to asset details view #160924
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
f83abca
to
96257f8
Compare
…rst-section-to-asset-details-view
@@ -98,6 +107,16 @@ const stories: Meta<AssetDetailsProps> = { | |||
memoryFree: 34359738368, | |||
}, | |||
overrides: { | |||
overview: { | |||
dataView: { |
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 couldn't find a good way to add the types here as I wanted to add this just to mock the data view in order to load the overview tab
@@ -38,6 +41,10 @@ export enum FlyoutTabIds { | |||
export type TabIds = `${FlyoutTabIds}`; | |||
|
|||
export interface TabState { | |||
overview?: { | |||
dateRange: StringDateRange; |
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.
This will change once we have the date picker for now the consumer of the component will set the time range
…rst-section-to-asset-details-view
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
|
||
return ( | ||
<> | ||
<EuiFlexGroup gutterSize="m" responsive={false} wrap={true} justifyContent="spaceBetween"> |
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.
<EuiFlexGroup gutterSize="m" responsive={false} wrap={true} justifyContent="spaceBetween"> | |
<EuiFlexGroup gutterSize="m" responsive={false} wrap justifyContent="spaceBetween"> |
@@ -0,0 +1,144 @@ | |||
/* |
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'm wondering what we could do to avoid duplicating this component. We should probably have a common folder. Not sure if that would work once we move the asset details to another plugin. But might be worth exploring the possibilities.
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.
Yeah, there are other dependencies still (LensWrapper
, buildCombinedHostsFilter
, TooltipContent
) and they are reused in other places. The host filter build helper is not really asset details specific and we can't move it because it should also live in infra
in the future (so when the component lives outside of infra
because we also don't want to get it from infra
once we move the asset details).
I will first pull the latest main
here and I will think about how to move more of the shared parts ( and at least for now we can reuse them from a common folder)
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 moved the dependencies and checked the component and there are still some differences in the filters, also the host component is connected to the host state while in asset view I changed it to accept props for the nodeName
, dateRange
, and dataView
. We can also have a common component and 2 wrappers to pass the data but then it's still something we need to rework when we move it, should we do that and have 3 components - one with the common parts and 2 wrappers (1 in asset details and 1 in host page)?
If we keep it in the asset details as a separate one we are also flexible to change it without affecting the host page. wdyt?
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 the way you did it. You moved the lens_wrapper
and other common components to common/visualizations
. For now, it looks great. In the future, if we realize these components are doing the same thing we can move them too
// FLAKY: https://github.com/elastic/kibana/issues/159368 | ||
// FLAKY: https://github.com/elastic/kibana/issues/159369 |
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.
Could we remove these comments too?
currentTimeRange={currentTimeRange} | ||
nodeName={node.name} | ||
nodeType={nodeType} | ||
onTabsStateChange={(tabId: TabIds) => onChange({ activeTabId: tabId })} |
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.
We could remove this prop, if we make the useTabSwitcher
fire the onTabsStateChange
event. In consequence we could remove the onChange
event handler
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.
Ok, I added the suggestion and used showTab here instead, but we are not getting rid of the onChange
function because it's used to handle the other state updates. I added this commit to apply the changes, I am not sure if I understood the idea correctly so we can discuss the changes there to make sure that it's correct.
if (onShowAllClick) { | ||
onShowAllClick(FlyoutTabIds.METADATA); | ||
} | ||
showTab(FlyoutTabIds.METADATA); |
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.
onShowAllClick
will in the end call the onTabsStateChange
. We could make the useTabSwitcher
do that:
export function useTabSwitcher({
initialActiveTabId,
onTabsStateChange,
}: {
initialActiveTabId?: TabIds;
onTabsStateChange?: TabsStateChangeFn;
}) {
const [activeTabId, setActiveTabId] = useState<TabIds | undefined>(initialActiveTabId);
// This set keeps track of which tabs content have been rendered the first time.
// We need it in order to load a tab content only if it gets clicked, and then keep it in the DOM for performance improvement.
const renderedTabsSet = useLazyRef(() => new Set([initialActiveTabId]));
const showTab = (tabId: TabIds) => {
renderedTabsSet.current.add(tabId); // On a tab click, mark the tab content as allowed to be rendered
setActiveTabId(tabId);
if (onTabsStateChange) {
onTabsStateChange({ activeTabId: tabId });
}
};
return {
activeTabId,
renderedTabsSet,
showTab,
};
}
We'd have to change the header.tsx
too, but might be a good idea to centralize this event in the context
reload: ( | ||
<EuiLink | ||
data-test-subj="infraMetadataReloadPageLink" | ||
onClick={() => window.location.reload()} |
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.
This will reload the whole page. Perhaps it's better to load only the flyout content instead
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.
That was added when we implemented the metadata fetching. The idea is to reload everything if this host for example doesn't exist anymore you can't get the metadata of the same host if you reload only the tab and it will go in circles - if we reload the page we can see that the host is not in the table.
We can think about some metadata improvements and do it both here and in the metadata component separately so it's not confusing.
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've forced the error here, and because we store in the URL the flyout state, which contains the active host.name
, I guess the user will continue in a loop if they reload the page
overview.mov
I'm wondering if just showing a message here would be enough. wdyt? Besides, window.location.reload()
forces a full page refresh
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.
Just realized that the Metadata tab does the same. 🤔
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.
Just realized that the Metadata tab does the same. 🤔
Yes, that's why I mentioned that is better to change both places and we can do it in a separate issue and implement there only a partial reload of the flyout ( so doing the call to get the metadata ).
I'm wondering if just showing a message here would be enough.
I briefly remember when we did it for the metadata tab and also other places - the UX idea behind it was to have an action together with the errors/warnings but in general, the idea was to be a link to reload the page instead of just a text to have something the user can click directly, I think this should not be shown very often though.
Also maybe in this case of the overview tab maybe we can customize it a bit so it's not so generic - showing the charts still (even if the metadata is not available) and then having a message that the metadata couldn't load (with try again later instead of reload link for example because we will still show something and it's not really only an error to force an action 🤔 ) wdyt?
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 briefly remember when we did it for the metadata tab and also other places - the UX idea behind it was to have an action together with the errors/warnings but in general, the idea was to be a link to reload the page instead of just a text to have something the user can click directly, I think this should not be shown very often though.
Yeah. providing users an option to recover from a failed state is good. Loading only the metadata and not reloading the whole page might be more effective.
Also maybe in this case of the overview tab maybe we can customize it a bit so it's not so generic - showing the charts still (even if the metadata is not available) and then having a message that the metadata couldn't load (with try again later instead of reload link for example because we will still show something and it's not really only an error to force an action 🤔 ) wdyt?
Agree! Blocking charts from loading if metadata fails to load seems weird. I like it.
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.
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 new issue for the refactoring added: #161367
hostOsVersion: i18n.translate( | ||
'xpack.infra.assetDetailsEmbeddable.overview.metadataHostOsVersionHeading', | ||
{ | ||
defaultMessage: 'Host os version', |
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.
Shouldn't it be OS? (I know it's os
in the mockup)
defaultMessage: 'Host os version', | |
defaultMessage: 'Host OS version', |
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.
Yeah, probably, changed 👍
<EuiFlexItem grow={false} key="metadata-link"> | ||
<EuiButtonEmpty | ||
data-test-subj="infraMetadataSummaryShowAllMetadataButton" | ||
onClick={onClick} | ||
size="s" | ||
flush="both" | ||
iconSide="right" | ||
iconType="sortRight" | ||
> | ||
<FormattedMessage | ||
id="xpack.infra.assetDetailsEmbeddable.metadataSummary.showAllMetadataButton" | ||
defaultMessage="Show all" | ||
/> | ||
</EuiButtonEmpty> |
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.
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.
Hmm I see the button is also smaller there, it's hard to align it as the empty button has this blue background on click. With a smaller size looks a bit better and closer to the mockup I think (I made it larger as I thought it will be more visible but most of the links are smaller so I will keep it consistent):
type: 'normalizedLoad1m', | ||
trendLine: true, | ||
backgroundColor: '#79AAD9', | ||
title: i18n.translate('xpack.infra.hostsViewPage.metricTrend.normalizedLoad1m.title', { |
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.
is xpack.infra.hostsViewPage
still correct in the flyout context? 🤔 . We'll need to change it for sure when we move these components to a new plugin
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, thanks, I renamed the ones in the translation folder but forget the others, I think now I renamed them everywhere in the overview folder
nodeType={nodeType} | ||
onTabsStateChange={(tabId: TabIds) => onChange({ activeTabId: tabId })} | ||
dataView={overrides?.overview?.dataView} | ||
dateRange={overrides?.overview?.dateRange!} |
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.
Let's pass the dateRange
as is and define a default date range in the Overview
component
dateRange={overrides?.overview?.dateRange!} | |
dateRange={overrides?.overview?.dateRange} |
Perhaps 15m range as default would work. wdyt?
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.
Sounds good, this is temporary as I already mentioned - until we have the date picker in the tab so for the time until that this is fine (then we will remove that from the tab and prefill the date picker and rely on it - also we can have the same concept with the default there as well)
@@ -168,7 +168,7 @@ export const Table = ({ loading, rows, onSearchChange, search, showActionsColumn | |||
interface ExpandableContentProps { | |||
values: string | string[] | undefined; | |||
} | |||
const ExpandableContent = (props: ExpandableContentProps) => { | |||
export const ExpandableContent = (props: ExpandableContentProps) => { |
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.
This is a good candidate to be moved to its own folder within asset_details
. Seems like a generic component
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 idea, I added a new folder for generic components and moved it.
import type { HostsLensMetricChartFormulas } from '../../../../common/visualizations'; | ||
import { LensWrapper } from '../../../../pages/metrics/hosts/components/chart/lens_wrapper'; | ||
import { buildCombinedHostsFilter } from '../../../../pages/metrics/hosts/utils'; | ||
import { TooltipContent } from '../../../../pages/metrics/hosts/components/metric_explanation/tooltip_content'; |
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.
We're importing this from ../../../../pages/metrics/hosts
. Perhaps this should be moved to a common folder
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.
Great job, Jenny! 🚀 Overview tab looks great. I've tested it and it works perfectly. Left some comments here. Let me know if you have any questions
nodeName: string; | ||
} | ||
|
||
export const KPIGrid = ({ nodeName, dataView, dateRange }: KPIGridProps) => { |
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.
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, thanks. Added 🙂
…rst-section-to-asset-details-view
044ac0b
to
ee3c1c2
Compare
…-add-overview-tab-with-the-first-section-to-asset-details-view
…rst-section-to-asset-details-view
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.
Thanks for the changes! Left some follow up comments. I'm not quite sure about the reload button, we could revisit this topic later if you feel like it's not for this PR.
currentTimeRange: MetricsTimeInput; | ||
nodeName: string; | ||
nodeType: InventoryItemType; | ||
onTabsStateChange?: (tabId: TabIds) => void; |
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.
This isn't used anymore. We can remove it now :)
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! Removed, thanks!
@@ -0,0 +1,144 @@ | |||
/* |
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 the way you did it. You moved the lens_wrapper
and other common components to common/visualizations
. For now, it looks great. In the future, if we realize these components are doing the same thing we can move them too
reload: ( | ||
<EuiLink | ||
data-test-subj="infraMetadataReloadPageLink" | ||
onClick={() => window.location.reload()} |
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've forced the error here, and because we store in the URL the flyout state, which contains the active host.name
, I guess the user will continue in a loop if they reload the page
overview.mov
I'm wondering if just showing a message here would be enough. wdyt? Besides, window.location.reload()
forces a full page refresh
gutterSize={'xs'} | ||
responsive={false} | ||
alignItems={'baseline'} | ||
wrap={true} |
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.
nit:
gutterSize={'xs'} | |
responsive={false} | |
alignItems={'baseline'} | |
wrap={true} | |
gutterSize="xs" | |
responsive={false} | |
alignItems="baseline" | |
wrap |
import type { DataViewField } from '@kbn/data-views-plugin/common'; | ||
import type { DataView } from '@kbn/data-views-plugin/public'; |
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.
Could we import both types from /public
?
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.
Oh yeah, DataViewField
is also available in public
, I guess the import helper got it from common
. Fixed 👍
reload: ( | ||
<EuiLink | ||
data-test-subj="infraMetadataReloadPageLink" | ||
onClick={() => window.location.reload()} |
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.
Just realized that the Metadata tab does the same. 🤔
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! 🚀
…-add-overview-tab-with-the-first-section-to-asset-details-view
import type { KPIProps } from './overview'; | ||
import type { StringDateRange } from '../../types'; | ||
|
||
const KPI_CHARTS: Array<Omit<KPIChartProps, 'loading' | 'subtitle'>> = [ |
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.
Would it be possible to reuse the config we have for the Hosts page for these KPIs? https://github.com/elastic/kibana/blob/main/x-pack/plugins/infra/public/pages/metrics/hosts/components/kpis/kpi_grid.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.
I moved the config to a common folder and now I use the same in both places 👍
export interface KPIChartProps { | ||
title: string; | ||
subtitle?: string; | ||
trendLine?: boolean; | ||
backgroundColor: string; | ||
type: HostsLensMetricChartFormulas; | ||
decimals?: number; | ||
toolTip: string; | ||
} | ||
|
||
const MIN_HEIGHT = 150; | ||
|
||
export const Tile = ({ | ||
title, | ||
type, | ||
backgroundColor, | ||
toolTip, | ||
decimals = 1, | ||
trendLine = false, | ||
nodeName, | ||
dateRange, | ||
dataView, | ||
}: KPIChartProps & KPIGridProps) => { |
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.
nit:
export interface KPIChartProps { | |
title: string; | |
subtitle?: string; | |
trendLine?: boolean; | |
backgroundColor: string; | |
type: HostsLensMetricChartFormulas; | |
decimals?: number; | |
toolTip: string; | |
} | |
const MIN_HEIGHT = 150; | |
export const Tile = ({ | |
title, | |
type, | |
backgroundColor, | |
toolTip, | |
decimals = 1, | |
trendLine = false, | |
nodeName, | |
dateRange, | |
dataView, | |
}: KPIChartProps & KPIGridProps) => { | |
export interface KPIChartProps, KPIGridProps { | |
title: string; | |
subtitle?: string; | |
trendLine?: boolean; | |
backgroundColor: string; | |
type: HostsLensMetricChartFormulas; | |
decimals?: number; | |
toolTip: string; | |
} | |
const MIN_HEIGHT = 150; | |
export const Tile = ({ | |
title, | |
type, | |
backgroundColor, | |
toolTip, | |
decimals = 1, | |
trendLine = false, | |
nodeName, | |
dateRange, | |
dataView, | |
}: KPIChartProps) => { |
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 moved the config and the type to a common folder so I now import both interfaces and still have KPIChartProps & KPIGridProps
is that fine?
…rst-section-to-asset-details-view
…rst-section-to-asset-details-view
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Closes #160376
Summary
This PR adds the initial version of the Overview tab including KPI tiles and metadata summary.
The storybook is not showing the lens charts as it will be hard because of the dependencies (we should consider if it is worth the effort to add them there in the future) The other parts look Ok:
Next steps 👣
There are still some parts that can be addressed separately when other parts are ready:
Testing
yarn storybook infra
to run the storybook and check bothPage
andFlyout