Skip to content

[ResponseOps][Connectors] Pager duty connector UI#171748

Merged
adcoelho merged 9 commits intoelastic:mainfrom
adcoelho:pager-duty-connector-ui
Nov 27, 2023
Merged

[ResponseOps][Connectors] Pager duty connector UI#171748
adcoelho merged 9 commits intoelastic:mainfrom
adcoelho:pager-duty-connector-ui

Conversation

@adcoelho
Copy link
Contributor

Fixes #170048

Summary

This PR adds support in the UI for the custom_details and links attributes in the Pagerduty connector.

Release Notes

PagerDuty connector now supports the links and custom_details attributes.

@adcoelho adcoelho added release_note:enhancement backport:skip This PR does not require backporting Feature:Actions Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.12.0 labels Nov 22, 2023
@adcoelho adcoelho self-assigned this Nov 22, 2023
@adcoelho adcoelho marked this pull request as ready for review November 23, 2023 14:12
@adcoelho adcoelho requested a review from a team as a code owner November 23, 2023 14:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Nice work! I tested and I found that the URLs are not working. It seems that the text is set to the URL and vice-versa

Screenshot 2023-11-24 at 3 11 21 PM

Also when you press the "Add link" button the links show an error even if I do not interact with the fields. Errors are better to be shown after user interaction as they happen with the other fields of the form. Lastly, I think it is better to not disable the "Add link" button on errors. I found it a bit disruptive. Better to follow the same pattern with the summary.

<TextFieldWithMessageVariables
index={index}
editAction={(key, value, i) => {
links[currentLinkIndex] = { ...links[currentLinkIndex], href: value };
Copy link
Member

Choose a reason for hiding this comment

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

I think we should avoid mutating directly the prop. The editAction will do it for you.

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 think we should avoid mutating directly the prop.

Initially, I had something like

const newLinks = [...links]
newLinks[currentLinkIndex] = { ...links[currentLinkIndex], href: value };
editAction('links', newLinks, index)

but playing around with it made no difference in the rerenders 😬

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by no difference?

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 thought changing the prop directly might affect the number of times the component was rerendered but in the end, they were the same. What exactly does it affect?

Copy link
Member

Choose a reason for hiding this comment

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

I can see some issues with mutating the props:

  1. React is not aware of the change of state
  2. We mutate an object that the parent component may use after using the child component
  3. We mutate an object that the code in the component assumes is immutable

@cnasikas
Copy link
Member

I forgot to add, is it possible to make the fields of the links take the full width of their share in the town (50%)?

Screenshot 2023-11-24 at 3 13 02 PM

@adcoelho
Copy link
Contributor Author

I tested and I found that the URLs are not working. It seems that the text is set to the URL and vice-versa

Ah, I switched them by mistake when playing with the editAction calls 😬

Also when you press the "Add link" button the links show an error even if I do not interact with the fields. Errors are better to be shown after user interaction as they happen with the other fields of the form.

I kinda did this on purpose. Isn't clicking "Add Link" an interaction with the links portion of the form?

Lastly, I think it is better to not disable the "Add link" button on errors.

Will do 👍

@cnasikas
Copy link
Member

cnasikas commented Nov 24, 2023

I kinda did this on purpose. Isn't clicking "Add Link" an interaction with the links portion of the form?

Hmm not for me but I would delegate this to @mdefazio 🙂.

@adcoelho
Copy link
Contributor Author

I kinda did this on purpose. Isn't clicking "Add Link" an interaction with the links portion of the form?

Hmm not for me but I would delegate this to @mdefazio 🙂.

Add link specifically calls the editAction callback to append a new item to the end of the list of links. This calls the validation that triggers the error.

But I can change it slightly to not highlight the input fields though.

From this Screenshot 2023-11-24 at 15 59 10
to this Screenshot 2023-11-24 at 15 58 27

@adcoelho
Copy link
Contributor Author

@mdefazio

Regarding the comment

Also when you press the "Add link" button the links show an error even if I do not interact with the fields. Errors are better to be shown after user interaction as they happen with the other fields of the form.

I made a video to show the current behavior.

Screen.Recording.2023-11-24.at.15.48.40.mov

What do you think?

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackConnectors 236 237 +1

Async chunks

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

id before after diff
stackConnectors 537.4KB 540.0KB +2.6KB

Page load bundle

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

id before after diff
stackConnectors 39.4KB 39.9KB +584.0B

History

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

cc @adcoelho

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Tested and is working as expected 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Actions release_note:enhancement Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.12.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Connectors][PagerDuty] Add support for links and custom_details in the UI

4 participants