[ObsUx][Infra] Add collapsible sections in the overview tab #175716

jennypavlova · 2024-01-26T17:25:20Z

Summary

This PR is a follow-up to #175558. It adds the active alerts count next to the alert section title (this will happen after the alerts widget is loaded) following the rules:

Default behaviour
No alerts at all ==> Collapse and say 'No active alerts'
No active alerts ==> Collapse and say 'No active alerts'
Active alerts ==> Expand fully

Collapsed
No alerts at all ==> Say 'No active alerts'
No active alerts ==> Say 'No active alerts'
Active alerts ==> say "X active alerts"

It adds a change in the AlertSummaryWidget to make it possible to get the alerts count after the widget is loaded using a new prop.

This PR also changes the alerts tab active alert count badge color on the hosts view to keep it consistent:

Before	After

Testing

Open hosts view and select a host with active alerts (flyout or full page)
- The alerts section should be expanded showing the alerts widget
  fd1a21035a5f)
- Collapse the alerts section by clicking on the title or the button:
Open hosts view and select a host without active alerts (flyout or full page)
- The alerts section should be collapsed showing the message 'No active alerts'

alerts_section_with_alerts_count.mov

…d-collapsible-sections-in-the-overview-tab

apmmachine · 2024-01-26T17:25:35Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

… there are no alerts

…b.com/jennypavlova/kibana into 175558-infra-add-collapsible-sections-in-the-overview-tab

…d-collapsible-sections-in-the-overview-tab

jennypavlova · 2024-02-05T17:35:40Z

/ci

…-overview-tab

elasticmachine · 2024-02-05T17:36:31Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

…-overview-tab

crespocarlos

Hey @jennypavlova , thanks for the adjustments. I have a couple of suggestions/questions:

I know we didn't have any designs for the message next to the alert title, but I'm wondering if we could use badges to display the alert count, to make it consistent with how we're going to display it in the Hosts Table:

https://docs.google.com/document/d/1XHcBdDjfJfA8sT1H1vHwG16qKxQCVbQ_Cy4zI3_zHaA/edit#heading=h.php0dgdhwjlp

When there is no active alerts, I wonder if we should still allow users to try to expand the section

no_active_alerts.mov

jennypavlova · 2024-02-07T11:19:39Z

I know we didn't have any designs for the message next to the alert title, but I'm wondering if we could use badges to display the alert count, to make it consistent with how we're going to display it in the Hosts Table

That's a good idea, thanks for sharing that. I tried to keep it simple for now before we get the design ready and discussed those messages with @roshan-elastic as a first iteration while we were waiting for the design. We can have either a badge (like those in the table) or the same style as the widget - I can try both and see how it looks :)

When there is no active alerts, I wonder if we should still allow users to try to expand the section

I know what you mean here - initially I showed the same "No alerts" message there but I don't think it's useful with the default closed section. Another case we miss here is when for example there are 5 recovered alerts the user should be still able to see the section there (even if there are no active alerts), so in short, we should still have the option to expand the section.
There are 2 cases when we have no alerts (any alerts) - when we don't have any rules or when the host is "healthy": If you ask me we can have something visual there that no active alerts are present or a prompt with a button to link to rule creation (in case of no rules) or both somehow - but IMO this should be handled in the alerts summary widget

jennypavlova · 2024-02-07T14:15:31Z

@roshan-elastic @crespocarlos So question about the active alerts message based on the discussion above and the alerts designs here I tried 2 versions of the closed section message with active alerts:

Same as the widget:
Same as table (badge):
Note: The current version is just text:

TBH I like the red color there but I can't decide which one fits better 😅 Wdyt?

BTW off-topic: but we can maybe also add an issue to convert the pink alert badge to red inside the hosts view tab so it will be also consistent with the design of the alerts feature (of course, if we are not trying to avoid red color for the specific case).

kpatticha · 2024-02-08T09:19:01Z

x-pack/plugins/infra/public/components/asset_details/tabs/overview/alerts.tsx

@@ -48,13 +53,23 @@ export const AlertsSummaryContent = ({
    [assetName, dateRange]
  );

+  const onLoaded = (alertsCount?: AlertsCount) => {
+    const hasActiveAlerts =
+      typeof alertsCount?.activeAlertCount === 'number' && alertsCount?.activeAlertCount > 0;


do we need typeof alertsCount?.activeAlertCount === 'number' ?

We have the type number | undefined so the first check is to make sure that it's a number and not undefined (Typescript will complain that it can be undefined if we remove the check) Do you think there is something wrong with checking that? We can also change it to alertsCount?.activeAlertCount && ... but I think checking the type is easy to understand and helps validate the activeAlertCount value type I think.

suggestion: you could do this

const { activeAlertCount = 0 } = alertsCount ?? {}; const hasActiveAlerts = activeAlertCount > 0; ...

crespocarlos · 2024-02-08T09:56:57Z

Thanks for looking into that @jennypavlova .

I like option 2, with the badge, both in the alerts section title and the alerts tab.

roshan-elastic · 2024-02-08T10:13:58Z

Hey @jennypavlova - I like the idea of being consistent between the alerts tab and the alerts section for sure.

I think we need UX here as I think it needs a little tweaking. Could we wait for @sileschristian to take a look a this?

roshan-elastic · 2024-02-08T10:19:35Z

Hey @jennypavlova - I like the idea of being consistent between the alerts tab and the alerts section for sure.

I think we need UX here as I think it needs a little tweaking. Could we wait for @sileschristian to take a look a this?

@jennypavlova @crespocarlos

Actually, given we're nearly at FF, @sileschristian still looks sick so I'd rather get this out and tweak it later.

Looking at APM, can we keep it consistent with how they do it (and we can tweak both if we need to once @sileschristian has had a look)?

Alerts in APM

Use APM alert style in hosts alert tab
Go with option Change of links to styles and js #2 in collapsed alert section state (but can you look at the alignment*?)

OK to go with that?

*the alignment looks a bit off

jennypavlova · 2024-02-08T11:51:48Z

@roshan-elastic @crespocarlos Thank you both!

Regarding the alignment - it was just an example, I didn't really look at the position just the item itself and thanks for spotting that, so I set a central alignment it now:

So I changed both as suggested:

Wdyt?

roshan-elastic · 2024-02-08T11:57:09Z

Good to go @jennypavlova - we can always update later!

Great work! :-D

jennypavlova · 2024-02-08T12:16:32Z

@roshan-elastic Thanks for checking that!
BTW I forgot the tooltip from the APM badge in the table and now I changed it to have the same styles:

jennypavlova · 2024-02-08T12:24:07Z

@crespocarlos Can you please take a look again - I updated the description and the screenshots :)

crespocarlos

Thanks for the changes @jennypavlova. Left a couple nits

crespocarlos · 2024-02-08T13:00:21Z

x-pack/plugins/infra/public/components/asset_details/tabs/overview/alerts.tsx

@@ -37,6 +39,9 @@ export const AlertsSummaryContent = ({
  const { featureFlags } = usePluginConfig();
  const [isAlertFlyoutVisible, { toggle: toggleAlertFlyout }] = useBoolean(false);
  const { overrides } = useAssetDetailsRenderPropsContext();
+  const [isAlertSectionOpen, setIsAlertSectionOpen] =


nit: This prop is not a boolean, we could name it collapsibleStatus, wdyt?

Makes sense, renamed ✅

crespocarlos · 2024-02-08T13:01:42Z

x-pack/plugins/infra/public/components/asset_details/tabs/overview/alerts.tsx

@@ -48,13 +53,23 @@ export const AlertsSummaryContent = ({
    [assetName, dateRange]
  );

+  const onLoaded = (alertsCount?: AlertsCount) => {
+    const hasActiveAlerts =
+      typeof alertsCount?.activeAlertCount === 'number' && alertsCount?.activeAlertCount > 0;


suggestion: you could do this

const { activeAlertCount = 0 } = alertsCount ?? {}; const hasActiveAlerts = activeAlertCount > 0; ...

crespocarlos

LGTM! Thanks @jennypavlova.

thomheymann

Code generally looks good. Small suggestion below.

thomheymann · 2024-02-08T13:38:45Z

x-pack/plugins/infra/public/components/asset_details/tabs/overview/alerts_closed_content.tsx

+  const shouldRenderAlertsClosedContent = typeof activeAlertCount === 'number';
+
+  if (!shouldRenderAlertsClosedContent) {
+    return null;
+  }


I think it should be the responsibility of the consumer to decide wether they want to render this component or not instead of having a component that doesn't render anything when no activeAlertCount prop has been passed in.

What do you think?

Thanks @thomheymann, you are right in general but the idea here is that before we set the activeAlertCount we don't want to display any message and the message should be shown once we have the activeAlertCount - so when it's a number.

Zacqary

ResponseOps changes LGTM

…d-collapsible-sections-in-the-overview-tab

kibana-ci · 2024-02-08T19:15:28Z

💚 Build Succeeded

Buildkite Build
Commit: 4747262

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1422	1423	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`infra`	1.3MB	1.3MB	+951.0B
`triggersActionsUi`	1.4MB	1.4MB	+46.0B
total			+997.0B

History

💔 Build #192250 failed 401bde1
💚 Build #191726 succeeded 9f9edec
💚 Build #191570 succeeded 0c98488
💚 Build #191452 succeeded 5e3a34f

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jennypavlova

…175716) Closes elastic#175989 ## Summary This PR is a follow-up to elastic#175558. It adds the active alerts count next to the alert section title (this will happen after the alerts widget is loaded) following the rules: Default behaviour No alerts at all ==> Collapse and say 'No active alerts' No active alerts ==> Collapse and say 'No active alerts' Active alerts ==> Expand fully Collapsed No alerts at all ==> Say 'No active alerts' No active alerts ==> Say 'No active alerts' Active alerts ==> say "X active alerts" It adds a change in the `AlertSummaryWidget` to make it possible to get the alerts count after the widget is loaded using a new prop. This PR also changes the alerts tab active alert count badge color on the hosts view to keep it consistent: | Before | After | | ------ | ------ | | <img width="242" alt="image" src="https://github.com/elastic/kibana/assets/14139027/85beec43-6522-4d58-a808-d3f7ec3e0759"> | <img width="263" alt="image" src="https://github.com/elastic/kibana/assets/14139027/43983493-3270-471a-8788-40ce38bed334"> | ## Testing - Open hosts view and select a host with active alerts (flyout or full page) - The alerts section should be expanded showing the alerts widget <img width="1920" alt="image" src="https://github.com/elastic/kibana/assets/14139027/f851c914-96cf-475f-bab3-dddc485405ec">fd1a21035a5f) - Collapse the alerts section by clicking on the title or the button: <img width="1917" alt="image" src="https://github.com/elastic/kibana/assets/14139027/e23e09ed-a781-4961-b592-6f13f539c316"> - Open hosts view and select a host without active alerts (flyout or full page) - The alerts section should be collapsed showing the message 'No active alerts' ![Image](https://github.com/elastic/obs-infraobs-team/assets/14139027/7077d3b3-c020-4be5-a3da-b46dda0d3ae0) https://github.com/elastic/kibana/assets/14139027/4058ed69-95f5-4b4c-8925-6680ac3791c1

jennypavlova added 4 commits January 25, 2024 20:02

Add collapsible sections in the asset details overview tab

53507a8

Merge branch 'main' of github.com:elastic/kibana into 175558-infra-ad…

b70bf99

…d-collapsible-sections-in-the-overview-tab

Update section id, add closed section content and no alerts case

636c00e

Add tests and fix test ids

bccf4cd

jennypavlova added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Jan 26, 2024

jennypavlova self-assigned this Jan 26, 2024

Change data test subjs

e8cdbe9

jennypavlova force-pushed the 175558-infra-add-collapsible-sections-in-the-overview-tab branch from 65278aa to cbaad15 Compare January 30, 2024 15:10

Try to use alerts count only in the alert widget and close section if…

2e9dcc8

… there are no alerts

jennypavlova force-pushed the 175558-infra-add-collapsible-sections-in-the-overview-tab branch from cbaad15 to 2e9dcc8 Compare January 30, 2024 15:11

jennypavlova mentioned this pull request Jan 31, 2024

[ObxUx][Infra] Alerts section: show alerts count and close the section if there are no alerts #175989

Closed

jennypavlova added 8 commits February 1, 2024 11:52

Update ids and alerts closed content

e820931

Refactoring: remove alerts customizations

e4d5d08

Merge branch 'main' into 175558-infra-add-collapsible-sections

d8f032b

Investigate a way to get alerts count

4b70650

Merge branch '175558-infra-add-collapsible-sections' of https://githu…

a739183

…b.com/jennypavlova/kibana into 175558-infra-add-collapsible-sections-in-the-overview-tab

Merge branch 'main' of github.com:elastic/kibana into 175558-infra-ad…

b4c377e

…d-collapsible-sections-in-the-overview-tab

Cleanup

87d718e

Add tests

5e3a34f

Merge branch 'main' into 175558-infra-add-collapsible-sections-in-the…

a91846d

…-overview-tab

jennypavlova marked this pull request as ready for review February 5, 2024 17:36

jennypavlova requested review from a team as code owners February 5, 2024 17:36

jennypavlova added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Feb 5, 2024

Merge branch 'main' into 175558-infra-add-collapsible-sections-in-the…

0c98488

…-overview-tab

crespocarlos reviewed Feb 7, 2024

View reviewed changes

kpatticha reviewed Feb 8, 2024

View reviewed changes

CR/Requested changes

aecd931

Align design with apm

6ee4927

jennypavlova requested review from crespocarlos and kpatticha February 8, 2024 12:25

crespocarlos reviewed Feb 8, 2024

View reviewed changes

CR changes

401bde1

crespocarlos approved these changes Feb 8, 2024

View reviewed changes

thomheymann reviewed Feb 8, 2024

View reviewed changes

thomheymann approved these changes Feb 8, 2024

View reviewed changes

Zacqary approved these changes Feb 8, 2024

View reviewed changes

jennypavlova added 2 commits February 8, 2024 18:28

Test id fix

9311caf

Merge branch 'main' of github.com:elastic/kibana into 175558-infra-ad…

4747262

…d-collapsible-sections-in-the-overview-tab

jennypavlova merged commit cd1f2b0 into elastic:main Feb 9, 2024
17 checks passed

jennypavlova deleted the 175558-infra-add-collapsible-sections-in-the-overview-tab branch February 9, 2024 10:06

kibanamachine added the v8.13.0 label Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ObsUx][Infra] Add collapsible sections in the overview tab #175716

[ObsUx][Infra] Add collapsible sections in the overview tab #175716

jennypavlova commented Jan 26, 2024 •

edited

Loading

apmmachine commented Jan 26, 2024

jennypavlova commented Feb 5, 2024

elasticmachine commented Feb 5, 2024

crespocarlos left a comment

jennypavlova commented Feb 7, 2024

jennypavlova commented Feb 7, 2024

kpatticha Feb 8, 2024 •

edited

Loading

jennypavlova Feb 8, 2024

crespocarlos Feb 8, 2024

jennypavlova Feb 8, 2024

crespocarlos commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024 •

edited

Loading

jennypavlova commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024

jennypavlova commented Feb 8, 2024

jennypavlova commented Feb 8, 2024

crespocarlos left a comment

crespocarlos Feb 8, 2024

jennypavlova Feb 8, 2024

crespocarlos Feb 8, 2024

crespocarlos left a comment

thomheymann left a comment

thomheymann Feb 8, 2024 •

edited

Loading

jennypavlova Feb 8, 2024

Zacqary left a comment

kibana-ci commented Feb 8, 2024

[ObsUx][Infra] Add collapsible sections in the overview tab #175716

[ObsUx][Infra] Add collapsible sections in the overview tab #175716

Conversation

jennypavlova commented Jan 26, 2024 • edited Loading

Summary

Testing

apmmachine commented Jan 26, 2024

🤖 GitHub comments

jennypavlova commented Feb 5, 2024

elasticmachine commented Feb 5, 2024

crespocarlos left a comment

Choose a reason for hiding this comment

jennypavlova commented Feb 7, 2024

jennypavlova commented Feb 7, 2024

kpatticha Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

jennypavlova Feb 8, 2024

Choose a reason for hiding this comment

crespocarlos Feb 8, 2024

Choose a reason for hiding this comment

jennypavlova Feb 8, 2024

Choose a reason for hiding this comment

crespocarlos commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024 • edited Loading

jennypavlova commented Feb 8, 2024

roshan-elastic commented Feb 8, 2024

jennypavlova commented Feb 8, 2024

jennypavlova commented Feb 8, 2024

crespocarlos left a comment

Choose a reason for hiding this comment

crespocarlos Feb 8, 2024

Choose a reason for hiding this comment

jennypavlova Feb 8, 2024

Choose a reason for hiding this comment

crespocarlos Feb 8, 2024

Choose a reason for hiding this comment

crespocarlos left a comment

Choose a reason for hiding this comment

thomheymann left a comment

Choose a reason for hiding this comment

thomheymann Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

jennypavlova Feb 8, 2024

Choose a reason for hiding this comment

Zacqary left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 8, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

jennypavlova commented Jan 26, 2024 •

edited

Loading

kpatticha Feb 8, 2024 •

edited

Loading

roshan-elastic commented Feb 8, 2024 •

edited

Loading

thomheymann Feb 8, 2024 •

edited

Loading