Fixed UI/UX issues: alerts delete confirmation, combobox behaviors #60703
Conversation
YulNaumenko
added
bug
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
…sues # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
...i/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx
Outdated
Show resolved
Hide resolved
| deleteAlert: ( | ||
| alert: Alert | ||
| ) => Promise<{ | ||
| successes: string[]; | ||
| errors: string[]; | ||
| }>; |
There was a problem hiding this comment.
I like that we're using only one api for deleting (using deleteAlerts for both multiple and single deletion requests), but I have a little nit picky thought. :)
It feels weird returning { successes: string[]; errors: string[]; } when the user calls deleteAlert, as it's only one id.
Could we perhaps change the signature to { successes?: string; errors?: string;}?
There was a problem hiding this comment.
If we're going to inline the type as is done here, agree w/Gidi that we could change the structure to match the semantic value. On the other hand, I can see this pattern of successes/errors being more widely used, and probably worth a separate type itself, and then having two different types - a "single" and "multiple" version - could get unwieldy.
x-pack/plugins/triggers_actions_ui/public/application/components/delete_modal_confirmation.tsx
Outdated
Show resolved
Hide resolved
...i/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx
Outdated
Show resolved
Hide resolved
| }): Promise<{ successes: string[]; errors: string[] }> { | ||
| const successes: string[] = []; | ||
| const errors: string[] = []; | ||
| await Promise.all(ids.map(id => http.delete(`${BASE_ALERT_API_PATH}/${id}`))).then( |
There was a problem hiding this comment.
I think if one delete fails, they will all report as failing. Seems like this needs to be restructured so that the delete's are wrapped in a function that never fails, but internally, it updates successes and errors, so that it can report a combination of successes and failures.
Seems survivable for 7.7, and should probably just be a new tech debt issue.
| deleteAlert: ( | ||
| alert: Alert | ||
| ) => Promise<{ | ||
| successes: string[]; | ||
| errors: string[]; | ||
| }>; |
There was a problem hiding this comment.
If we're going to inline the type as is done here, agree w/Gidi that we could change the structure to match the semantic value. On the other hand, I can see this pattern of successes/errors being more widely used, and probably worth a separate type itself, and then having two different types - a "single" and "multiple" version - could get unwieldy.
…sues # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/sections/actions_connectors_list/components/actions_connectors_list.tsx # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts
…sues # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…lastic#60703) * Fixed UI/UX issues: alerts delete confirmation * Fixed 4. Popover disappears when clearing the field selector * Fixed tests * Fixed due to comments * fixed tests * Fixed test # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
kibanamachine
added
the
backport missing
…lastic#60703) * Fixed UI/UX issues: alerts delete confirmation * Fixed 4. Popover disappears when clearing the field selector * Fixed tests * Fixed due to comments * fixed tests * Fixed test # Conflicts: # x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/alerts.ts
kibanamachine
removed
the
backport missing
* master: (39 commits) [APM]Create custom link from Trace summary (elastic#59648) [ML] Fixing app clean up (elastic#60853) [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734) [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016) Skip failing test [Uptime]Update fetch effect failed action handling (elastic#60742) [npm] upgrade elastic/maki (elastic#60829) [Uptime] Add Settings Page (elastic#53550) [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836) [Index management] Re-enable index template tests (elastic#60780) Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703) [SIEM] Fix patching of ML Rules (elastic#60830) [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477) [Alerting] fix flaky test for index threshold grouping (elastic#60792) [SIEM][Detection Engine] Adds test scripts for machine learning feature Flatten child api response for resolver (elastic#60810) Change "url" to "urls" in APM agent instructions (elastic#60790) [DOCS] Updates API requests and examples (elastic#60695) [SIEM] [Cases] Create case from timeline (elastic#60711) [Lens] Resetting a layer generates new suggestions (elastic#60674) ...
Resolve #58364 and #60204