[Actionable Observability] Display action connector icon in o11y rule details page #132026

mgiota · 2022-05-11T13:00:07Z

Fixes #132220
Fixes #132580

…nd write some tests

mgiota · 2022-05-18T09:27:08Z

@elasticmachine merge upstream

katrin-freihofner

LGTM

x-pack/plugins/observability/public/pages/rule_details/components/actions.tsx

fkanout

@mgiota there is size inconsistency between the app icons. IBM logo is too small, xMatter icon too big, swimlane is truncated. Most likely it's related to SVG view box size, I would say.

gmmorris

LGTM, as this is just an export of a fairly generic component and I don't want to block @mgiota .

That said - I was wondering whether suspendedComponentWithProps is so generic that it actually shouldn't be in TriggerActionsUI at all? 🤔
Any thoughts @elastic/response-ops-ram ?

mikecote

ResponseOps changes LGTM

JiaweiWu

LGTM!

JiaweiWu · 2022-05-18T23:14:19Z

@gmmorris Yea I think the implementation is so trivial that it might make sense as a guideline (suspended components must use the EUI loading indicator) rather than a reusable component. I would even argue that having it as a reusable component add an unnecessary layer of abstraction.

mgiota · 2022-05-19T07:08:09Z

@fkanout I tried a few things with eui styled components (wrapping a div around the icon and applying some custom styles), but the problem still persists. Regarding xmatters, I noticed that xLarge is defined directly in the logo.tsx https://github.com/elastic/kibana/blob/main/x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/xmatters/logo.tsx#L15, which I think should be removed anyway.

Regarding the smaller icons you mentioned, I am not sure if we can do anything about it, other than changing the svgs, but that would require more testing on the rest places icons appear.

Do you have any suggestions how we can fix the above problem with xmatters? Current Github issue was mainly about using the actionTypeRegistry to retrieve the existing logos. Do you think we can merge this one and fix the UI problem in a separate Github issue?

fkanout · 2022-05-19T10:29:42Z

@mgiota, I know dealing with SVG is not the easiest thing 😮‍💨 . I will take a look.

However, I wonder how they are size-standardized in the connectors' panel 🤔 ?

mgiota · 2022-05-19T11:50:34Z

@fkanout My first guess is that these icons are of size="l" or size="xl", so resizing svgs to the biggest dimension would make all of them look the same. The other way around causes troubles, having some bigger svgs and trying to make them smaller, which is exactly the challenge we face right now. In our code we have size="m" for the EuiIcon and all icons apart from xmatters get resized and look kind of ok. But xmatters has className="euiIcon euiIcon--xLarge euiCard__icon" in the logo.tsx. Even removing this didn't fix the issue. I will definitely dig into it and hopefully find a solution. Let me know if you find something.

UPDATE Yep my assumption is correct. In the connector's panel they specify their icons as size="xl". I changed to size="m" as we have in our code and the exact same issue is reproducible. Good news is that we know for sure that only xmatters is problematic.
.

Accordingly if I change our icons to size="xl" they all look fine. I am not suggesting of course to make them that big (I am not a fan of big icons anyway). We just need a new svg for xmatters. I summarized the needed steps in your suggested solution below.

fkanout · 2022-05-19T12:34:57Z

x-pack/plugins/observability/public/pages/rule_details/components/actions.tsx

-      {actions.map((action) => (
-        <>
+      {actions.map(({ actionTypeId, name }) => (
+        <React.Fragment key={actionTypeId}>
          <EuiFlexGroup alignItems="baseline">


Suggested change

<EuiFlexGroup alignItems="baseline">

<EuiFlexGroup alignItems="center">

<EuiFlexItem

style={{

position: 'relative',

width: '26px',

height: '26px',

}}

grow={false}

>

<EuiIcon

style={{

position: 'absolute',

height: '100%',

width: '100%',

left: 0,

top: 0,

}}

size="m"

type={getActionIconClass(actionTypeId) ?? 'apps'}

/>

</EuiFlexItem>

@mgiota, It's not perfect, but it seems a bit better. feel free to add it if you think so

@fkanout Great, I am back to this PR and I am gad to see it is already looks better, more symmetrical. I can take it from here and do a bit of refactoring regarding using import { euiStyled } from '@kbn/kibana-react-plugin/common'; to not do the styling inline. But this doesn't fix the root of the problem, which is the xmatters svg icon.

@fkanout @katrin-freihofner Here's what I suggest:

Remove className="euiIcon euiIcon--xLarge euiCard__icon" from xmatters logo.tsx file

Decide on the width and height of the custom container. I think 26px is a little bit big. @katrin-freihofner What do you think?

Fix xmatters svg

Here's how it looks with the first 2 changes. Step 3 is missing and I think it should look fine.

@katrin-freihofner Could you provide us with a new SVG for xmatters? Here you can find the existing svg. I started playing and changing viewBox etc, but no success.

You can also read my comment above for more context #132026 (comment)

Good news, I played a bit more with the xmatters svg file and I fixed the issue. Step 2 (custom dimensions) is not needed anymore, since xmatters behaves well now with the specified size="m" 😄

Sidenote: Alignment is a bit off, but I guess this is out of scope of this PR, since I noticed more alignment issues anyway.

@mgiota, great work! 👏🏻

@fkanout Small issues surfaced with this PR, good that it looks much better now! Your approval is still needed for this PR.

…r_iconClass

…-fix'

…r_iconClass

mgiota · 2022-05-19T20:44:32Z

@fkanout I fixed the issue. Could you do one more round of testing?

I created a ticket to track the fix we did for xmatters icon #132580, since this PR fixes current Rules and Connectors page as well. Regarding smaller icons they also don't look perfect there as well.

Current status Rules and connectors

After our fix

@katrin-freihofner Adding custom width and height in our page is not going to fix the issue with smaller icons. The svg icons need to be changed as well. I suggest we create a new ticket and we fix this seperately. This is definitely out of scope of current issue.

UPDATE: Here's the new issue to fix smaller logo icons.

mgiota · 2022-05-20T08:47:37Z

@elasticmachine merge upstream

fkanout

LGTM. The icons render a lot better. The Swimlane icon seems to need some fix in separate PR as it's out of the scope of this one.

mgiota · 2022-05-20T09:48:18Z

@fkanout Yep exactly! Here's the new issue regarding taking care of smaller svg icons #132591

kibana-ci · 2022-05-20T09:48:26Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`triggersActionsUi`	370	373	+3

Async chunks

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

id	before	after	diff
`observability`	452.3KB	452.5KB	+183.0B
`triggersActionsUi`	786.3KB	786.4KB	+16.0B
total			+199.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	115.5KB	115.5KB	+62.0B

Unknown metric groups

API count

id	before	after	diff
`triggersActionsUi`	384	387	+3

History

💚 Build #46259 succeeded e3701d5
💚 Build #46226 succeeded dde0c77
💚 Build #46181 succeeded 2e9e096
💚 Build #45642 succeeded 190f5ae
💔 Build #45613 failed 8b6c669
💚 Build #45103 succeeded 292a67b

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

cc @mgiota

mgiota self-assigned this May 16, 2022

mgiota added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.3.0 release_note:skip Skip the PR/issue when compiling release notes labels May 16, 2022

mgiota force-pushed the action_connector_iconClass branch from 5d14f58 to 584b174 Compare May 16, 2022 18:22

get iconClass from actionRegistry

292a67b

mgiota force-pushed the action_connector_iconClass branch from 584b174 to 292a67b Compare May 16, 2022 20:57

mgiota marked this pull request as ready for review May 17, 2022 06:38

mgiota requested a review from a team as a code owner May 18, 2022 07:01

mgiota force-pushed the action_connector_iconClass branch 2 times, most recently from c702f31 to a4be29e Compare May 18, 2022 07:10

use suspendedComponentWithProps when iconClass is a react component a…

8b6c669

…nd write some tests

mgiota force-pushed the action_connector_iconClass branch from a4be29e to 8b6c669 Compare May 18, 2022 07:36

mgiota requested a review from fkanout May 18, 2022 07:36

mgiota changed the title ~~[Actionable Observability] get action connector iconClass for actionTypeRegistry~~ [Actionable Observability] Display action connector icon in o11y rule details page May 18, 2022

Merge branch 'main' into action_connector_iconClass

190f5ae

katrin-freihofner reviewed May 18, 2022

View reviewed changes

x-pack/plugins/observability/public/pages/rule_details/components/actions.tsx Outdated Show resolved Hide resolved

x-pack/plugins/observability/public/pages/rule_details/components/actions.tsx Outdated Show resolved Hide resolved

fkanout requested changes May 18, 2022

View reviewed changes

gmmorris approved these changes May 18, 2022

View reviewed changes

mikecote approved these changes May 18, 2022

View reviewed changes

JiaweiWu approved these changes May 18, 2022

View reviewed changes

mgiota requested a review from fkanout May 19, 2022 07:08

fkanout reviewed May 19, 2022

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into action_connecto…

d006ac6

…r_iconClass

mgiota requested a review from katrin-freihofner May 19, 2022 15:05

kibanamachine and others added 2 commits May 19, 2022 15:31

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

2e9e096

…-fix'

Merge branch 'main' of github.com:elastic/kibana into action_connecto…

dde0c77

…r_iconClass

mgiota requested a review from fkanout May 19, 2022 20:13

fix xmatters svg icon

e3701d5

apply design feedback

cf1e1be

mgiota enabled auto-merge (squash) May 20, 2022 08:38

Merge branch 'main' into action_connector_iconClass

0611ad9

mgiota mentioned this pull request May 20, 2022

[Rules Connectors page] Fix issue with xmatters icon #132580

Closed

fkanout approved these changes May 20, 2022

View reviewed changes

mgiota mentioned this pull request May 20, 2022

[Action Connectors] make ibm and service now svg icons bigger #132591

Open

mgiota merged commit de90ea5 into elastic:main May 20, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 20, 2022

mgiota deleted the action_connector_iconClass branch September 29, 2023 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Actionable Observability] Display action connector icon in o11y rule details page #132026

[Actionable Observability] Display action connector icon in o11y rule details page #132026

mgiota commented May 11, 2022 •

edited

Loading

mgiota commented May 18, 2022

katrin-freihofner left a comment

fkanout left a comment •

edited

Loading

gmmorris left a comment

mikecote left a comment

JiaweiWu left a comment

JiaweiWu commented May 18, 2022

mgiota commented May 19, 2022 •

edited

Loading

fkanout commented May 19, 2022 •

edited

Loading

mgiota commented May 19, 2022 •

edited

Loading

fkanout May 19, 2022

mgiota May 19, 2022 •

edited

Loading

mgiota May 19, 2022 •

edited

Loading

mgiota May 19, 2022 •

edited

Loading

fkanout May 20, 2022

mgiota May 20, 2022

mgiota commented May 19, 2022 •

edited

Loading

mgiota commented May 20, 2022

fkanout left a comment

mgiota commented May 20, 2022

kibana-ci commented May 20, 2022

API count

-          <EuiFlexGroup alignItems="baseline">
+          <EuiFlexGroup alignItems="center">
+            <EuiFlexItem
+              style={{
+                position: 'relative',
+                width: '26px',
+                height: '26px',
+              }}
+              grow={false}
+            >
+              <EuiIcon
+                style={{
+                  position: 'absolute',
+                  height: '100%',
+                  width: '100%',
+                  left: 0,
+                  top: 0,
+                }}
+                size="m"
+                type={getActionIconClass(actionTypeId) ?? 'apps'}
+              />
+            </EuiFlexItem>

[Actionable Observability] Display action connector icon in o11y rule details page #132026

[Actionable Observability] Display action connector icon in o11y rule details page #132026

Conversation

mgiota commented May 11, 2022 • edited Loading

mgiota commented May 18, 2022

katrin-freihofner left a comment

Choose a reason for hiding this comment

fkanout left a comment • edited Loading

Choose a reason for hiding this comment

gmmorris left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

JiaweiWu left a comment

Choose a reason for hiding this comment

JiaweiWu commented May 18, 2022

mgiota commented May 19, 2022 • edited Loading

fkanout commented May 19, 2022 • edited Loading

mgiota commented May 19, 2022 • edited Loading

fkanout May 19, 2022

Choose a reason for hiding this comment

mgiota May 19, 2022 • edited Loading

Choose a reason for hiding this comment

mgiota May 19, 2022 • edited Loading

Choose a reason for hiding this comment

mgiota May 19, 2022 • edited Loading

Choose a reason for hiding this comment

fkanout May 20, 2022

Choose a reason for hiding this comment

mgiota May 20, 2022

Choose a reason for hiding this comment

mgiota commented May 19, 2022 • edited Loading

mgiota commented May 20, 2022

fkanout left a comment

Choose a reason for hiding this comment

mgiota commented May 20, 2022

kibana-ci commented May 20, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

mgiota commented May 11, 2022 •

edited

Loading

fkanout left a comment •

edited

Loading

mgiota commented May 19, 2022 •

edited

Loading

fkanout commented May 19, 2022 •

edited

Loading

mgiota commented May 19, 2022 •

edited

Loading

mgiota May 19, 2022 •

edited

Loading

mgiota May 19, 2022 •

edited

Loading

mgiota May 19, 2022 •

edited

Loading

mgiota commented May 19, 2022 •

edited

Loading