[Cases] Adding deprecated icon to additional actions dropdown selectors #115287
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathan-buttner
added
v8.0.0
release_note:skip
...ggers_actions_ui/public/application/sections/action_connector_form/action_type_form.test.tsx
Outdated
Show resolved
Hide resolved
...ggers_actions_ui/public/application/sections/action_connector_form/action_type_form.test.tsx
Outdated
Show resolved
Hide resolved
...ggers_actions_ui/public/application/sections/action_connector_form/action_type_form.test.tsx
Outdated
Show resolved
Hide resolved
...ggers_actions_ui/public/application/sections/action_connector_form/action_type_form.test.tsx
Outdated
Show resolved
Hide resolved
...ggers_actions_ui/public/application/sections/action_connector_form/action_type_form.test.tsx
Outdated
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/action_connector_form/action_type_form.tsx
Outdated
Show resolved
Hide resolved
...s/triggers_actions_ui/public/application/sections/action_connector_form/action_type_form.tsx
Outdated
Show resolved
Hide resolved
| @@ -86,24 +88,14 @@ export const AddConnectorInline = ({ | |||
| ); | |||
|
|
|||
| useEffect(() => { | |||
| if (connectors) { | |||
There was a problem hiding this comment.
A couple things about these changes. This logic was setting state to keep track of whether it had some connectors after the filter. I moved the filter functionality to a helper that is used in a couple places (getEnabledAndConfiguredConnectors).
The helper will return an empty array if there were no connectors passed in, so we don't need the if (connectors) anymore.
Once it set the setConnectorOptionsList, it would also set the errors.
The errors are only used when connectorOptionsList is not an empty array (aka in the connectorsDropdown). The connectorsDropdown is also only rendered when connectorOptionsList is not empty. Therefore, I believe we can remove most of the state, pass the error array directly into the connectorsDropdown component, and use a boolean to determine if we had any connectors instead.
| error={errors} | ||
| isInvalid={errors.length > 0} | ||
| error={connectorDropdownErrors} | ||
| isInvalid |
There was a problem hiding this comment.
I could be missing something but I believe the only time this component is render is when there would be errors. So we don't need the check.
x-pack/plugins/triggers_actions_ui/public/application/sections/common/connectors.ts
Outdated
Show resolved
Hide resolved
semd
reviewed
Oct 18, 2021
x-pack/plugins/triggers_actions_ui/public/application/sections/common/connectors.ts
Outdated
Show resolved
Hide resolved
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form.action_form action_form in alert renders available connectors for the selected action typeStandard OutStack TraceKibana Pipeline / jest / Jest Tests.x-pack/plugins/triggers_actions_ui/public/application/sections/action_connector_form.action_form action_form in alert renders only preconfigured connectors for the selected preconfigured action typeStandard OutStack TraceKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details·ts.Actions and Triggers app Alert Details Edit alert button should open edit alert flyoutStandard OutStack Traceand 1 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
|
@elasticmachine merge upstream |
…ner/kibana into connectors-deprecated-icon
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
I think we will need to add truncation regardless to avoid this situation for any long connector name. Some questions, but I think what I'm getting at is that we don't need either appended texts:
By modifying this, don't we lose the option to have search in this dropdown? Is it a case of being able to have one or the other (an icon vs allowing search)? |
|
@mdefazio thanks for the feedback.
Good idea, I'll add that.
Good point. I don't believe users can edit preconfigured connectors name. Is that right @YulNaumenko ?
Yeah we probably don't need the Cases looks similar:
PR: #114234 Should I remove it in both places?
Good point, I had totally missed that we lose the ability to search. I tried getting an icon to show up using the If you have an example I'm happy to give it another shot. Or if there's another EUI component that would be a better fit, let me know. |
…ner/kibana into connectors-deprecated-icon
+1 on this
Correct, this is a part of the configuration.
I prefer to have the icon to have less busy UI with the text. And +1 to be consistent everywhere.
Yes, lets have the icon with tooltip only.
Great catch! That needs to be fixed. |
|
@jonathan-buttner, regarding the issue I've opened to fix the deprecation for preconfigured connectors by exposing the config values. The team decided to postpone that functionality till we get the better research on the impact and will get some telemetry around how many customers using preconfigured ServiceNow connectors to prioritize that feature. |
|
Adding in our summary from the Slack thread: |
jonathan-buttner
force-pushed
the
connectors-deprecated-icon
branch
from
d2cf023 to
0ed891f
Compare
jonathan-buttner
commented
Dec 3, 2021
There was a problem hiding this comment.
The latest commit fixes:
- Reverts back to using a combo box to provide search capability
- When a deprecated connector is selected we prepend the warning icon
Example prepended icon
- Fixes the overflow of text in a long connector title
Example long connector title
- Don't show the icon in the dropdown list, but show
(deprecated)
Example deprecated text
| @@ -214,90 +175,73 @@ export const ActionTypeForm = ({ | |||
| <> | |||
| {actionGroups && selectedActionGroup && setActionGroupIdByIndex && ( | |||
| <> | |||
| <EuiFlexGroup component="div"> | |||
| <EuiFlexItem grow={true}> | |||
There was a problem hiding this comment.
Caroline asked that I remove the FlexGroup and Item because they're only useful when you have multiple items within a group.
| @@ -214,90 +175,73 @@ export const ActionTypeForm = ({ | |||
| <> | |||
| {actionGroups && selectedActionGroup && setActionGroupIdByIndex && ( | |||
| <> | |||
| <EuiFlexGroup component="div"> | |||
| <EuiFlexItem grow={true}> | |||
| <EuiFormControlLayout | |||
There was a problem hiding this comment.
Caroline recommended that I move the prepend prop from this component straight into the super select and and remove the EuiFormControlLayout as it wasn't needed. This was also causing rounded edges which wasn't consistent with the prepend on the combobox we use below
| <EuiSpacer size="l" /> | ||
| </> | ||
| )} | ||
| <EuiFlexGroup component="div"> |
There was a problem hiding this comment.
Same thing here, we didn't need the group and item because there was only one of them and they're only needed when aligning things horizontally.
This also addresses the overflow of text for the ComboBox and super selector that we were seeing.
| } | ||
|
|
||
| setIsEmptyActionId(!!emptyActionsIds.find((emptyId: string) => actionItem.id === emptyId)); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
|
|
||
| const connectorsDropdown = ( | ||
| <EuiFlexGroup component="div"> |
There was a problem hiding this comment.
Same thing here don't need the group and item.
mdefazio
approved these changes
Dec 3, 2021
|
@elasticmachine merge upstream |
jonathan-buttner
commented
Dec 6, 2021
| @@ -135,6 +135,12 @@ export interface ActionTypeModel<ActionConfig = any, ActionSecrets = any, Action | |||
| > | |||
| > | null; | |||
| actionParamsFields: React.LazyExoticComponent<ComponentType<ActionParamsProps<ActionParams>>>; | |||
| customConnectorSelectItem?: { | |||
There was a problem hiding this comment.
@YulNaumenko I split this up into two different operations because we only display the icon (aka getComponent) after the row has been selected, but we display the text in the dropdown of the combo box. Let me know if you have a better idea. I'm happy to change it.
There was a problem hiding this comment.
Thank you for the explanation. I think we better create the proper reusable interface instead of providing that details coupled to the ActionTypeModel. But the rest looks good to me.
…ner/kibana into connectors-deprecated-icon
YulNaumenko
approved these changes
Dec 7, 2021
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
jonathan-buttner
added
backport:skip
TinLe
pushed a commit
to TinLe/kibana
that referenced
this pull request
Dec 22, 2021
…rs (elastic#115287) * adding deprecated icon to the other actions list * Adding deprecated text to list view * Each action type can render the dropdown row * Refactoring and fixing todos * Fixing jest tests * Adding and fixing other tests * Fixing functional test * Fixing tests * Adjusting text to match cases * Fixing tests * Addressing pr feedback * Renaming connector dropdown to selection * Fixing type errors * Fixing type error * Fixing translation error * Fixing test * Addressing ux feedback and using ComboBox * Extracting customConnectorSelectItem to an interface Co-authored-by: Kibana Machine <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue: #114097
This PR adds the deprecated icon to the dropdown selector when creating rules with the triggers_actions_ui plugin.
To get the icon to show up I had to refactor it to use a super selector instead of a combobox.
I also refactored the code to use a single component since it needed to be added in two places:
connector_add_inlinecomponentConnector list view
Adding table api connector to rule
This PR doesn't include the copy updates outlined here: https://github.com/elastic/security-team/issues/1938