-
-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement optional json payload and template #4752
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
...grations/IntegrationForm/IntegrationParameters/IntegrationParameter/IntegrationParameter.tsx
Outdated
Show resolved
Hide resolved
...IntegrationForm/IntegrationParameters/IntegrationParameter/IntegrationParameterTextField.tsx
Outdated
Show resolved
Hide resolved
...grations/IntegrationForm/IntegrationParameters/IntegrationParameter/IntegrationParameter.tsx
Outdated
Show resolved
Hide resolved
...onForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
Outdated
Show resolved
Hide resolved
...onForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
Outdated
Show resolved
Hide resolved
...onForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
Outdated
Show resolved
Hide resolved
...onForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
Outdated
Show resolved
Hide resolved
...onForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general thought is that this feels slightly overkill. If the body template is optional, do we really need the dropdown that shows/hides it? Because it feels like we could just leave it there, similar to what we do for the webhook addon. We could still hide it behind the feature flag if we wanted.
I feel like this would simplify this PR and feature drastically.
In the other hand, if we really want this dropdown visibility behavior, perhaps it should be a different field type, other than textfield?
Other than that this feels fine, but I would still like us to think a bit about this before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
About the changes
Adds optional support for specifying JSON templates for datadog message payload
Important files
frontend/src/component/integrations/IntegrationForm/IntegrationParameters/IntegrationParameter/IntegrationParameterEnableWithDropdown.tsx
- a new component comprising of a text field and a dropdown menusrc/lib/addons/datadog.ts
- Where the integration is taking placeDiscussion points