Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Show service group icon only for service groups #131138

Merged
merged 8 commits into from
May 20, 2022
Merged

Show service group icon only for service groups #131138

Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7b2066
Show service group icon when there are service groups
kpatticha Apr 28, 2022
301f629
Fix fix errors
kpatticha Apr 29, 2022
f6be5e7
Merge branch 'main' of github.com:elastic/kibana into 128229-service-…
kpatticha May 11, 2022
7ff2bc9
Remove additional request and display icon only for the service groups
kpatticha May 11, 2022
41b619f
Merge branch 'main' of github.com:elastic/kibana into 128229-service-…
kpatticha May 17, 2022
29a8250
Revert "Remove additional request and display icon only for the serv…
kpatticha May 17, 2022
8046a02
Add dependencies
kpatticha May 20, 2022
19c2467
Merge branch 'main' into 128229-service-groups-icon
kibanamachine May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions x-pack/plugins/apm/public/components/routing/templates/service_group_template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
EuiFlexItem,
EuiButtonIcon,
EuiLoadingContent,
EuiLoadingSpinner,
} from '@elastic/eui';
import React from 'react';
import { i18n } from '@kbn/i18n';
Expand All @@ -19,7 +20,7 @@ import {
KibanaPageTemplateProps,
} from '@kbn/kibana-react-plugin/public';
import { enableServiceGroups } from '@kbn/observability-plugin/public';
import { useFetcher } from '../../../hooks/use_fetcher';
import { useFetcher, FETCH_STATUS } from '../../../hooks/use_fetcher';
import { ApmPluginStartDeps } from '../../../plugin';
import { useApmRouter } from '../../../hooks/use_apm_router';
import { useAnyOfApmParams } from '../../../hooks/use_apm_params';
Expand Down Expand Up @@ -60,8 +61,19 @@ export function ServiceGroupTemplate({
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const { data: serviceGroupsData, status: serviceGroupsStatus } = useFetcher(
(callApmApi) => {
if (!serviceGroupId && isServiceGroupsEnabled) {
return callApmApi('GET /internal/apm/service-groups');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to fetch data? I figure we can hide the icon if "all services" is selected, and show it in all other cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be an option to avoid the additional request.

@formgeist what do you think from the UI point of view? it will look like this

Screen.Recording.2022-04-29.at.14.22.25.mov

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's confusing if the user clicks "All services" group and we still show the "Services" title. cc @boriskirov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpatticha We could use isServiceGroupsEnabled to decide whether to show "Services" or "All services" no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed the naming in this PR #131381

}
},
// eslint-disable-next-line react-hooks/exhaustive-deps
kpatticha marked this conversation as resolved.
Show resolved Hide resolved
[]
);

const serviceGroupName = data?.serviceGroup.groupName;
const loadingServiceGroupName = !!serviceGroupId && !serviceGroupName;
const hasServiceGroups = !!serviceGroupsData?.serviceGroups.length;
const serviceGroupsLink = router.link('/service-groups', {
query: { ...query, serviceGroup: '' },
});
Expand All @@ -74,15 +86,22 @@ export function ServiceGroupTemplate({
justifyContent="flexStart"
responsive={false}
>
<EuiFlexItem grow={false}>
<EuiButtonIcon
iconType="layers"
color="text"
aria-label="Go to service groups"
iconSize="xl"
href={serviceGroupsLink}
/>
</EuiFlexItem>
{serviceGroupsStatus === FETCH_STATUS.LOADING && (
<EuiFlexItem grow={false}>
<EuiLoadingSpinner size="l" />
</EuiFlexItem>
)}
{(serviceGroupId || hasServiceGroups) && (
<EuiFlexItem grow={false}>
<EuiButtonIcon
iconType="layers"
color="text"
aria-label="Go to service groups"
iconSize="xl"
href={serviceGroupsLink}
/>
</EuiFlexItem>
)}
<EuiFlexItem grow={false}>
{loadingServiceGroupName ? (
<EuiLoadingContent lines={2} style={{ width: 180, height: 40 }} />
Expand Down