Disable the Query parameter box in Custom webhook. #72
Disable the Query parameter box in Custom webhook. #72
Conversation
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.
@ricardobessa1 Thank you for this PR , few small comments. Let me know your thoughts.
return ( | ||
<div> | ||
<URLInfo type={type} values={values} /> | ||
<QueryParamsEditor type={type} queryParams={values[type].queryParams} isEnabled={isQueryParamsEnabled} /> |
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.
I think we should now move this under URLInfo
as it is part of it.
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.
Agreed, makes more sense move QueryParamsEditor
under URLInfo
because its behavior depends of one value inside URLInfo
.
name={`${type}.queryParams`} | ||
addButtonText="Add parameter" | ||
removeButtonText="Remove parameter" | ||
onRenderKeyField={handleRenderKeyField} |
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.
how about if we could use isEnabled
directly here instead of passing it to <AttributeEditor/>
. ?
onRenderKeyField={(fieldName, index) => handleRenderKeyField(fieldName, inedex, isEnabled)}
This way we can avoid passing props to component tree and make <AttributeEditor/>
more generic.
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.
I can use the isEnabled
directly on QueryParamsEditor
declaring the functions passed to onRenderKeyField
and onRenderValueField
as:
const handleKeyField = (fieldName, index) => (handleRenderKeyField(fieldName, index, isEnabled));
const handleValueField = (fieldName, index) => (handleRenderValueField(fieldName, index, isEnabled));
replacing
onRenderKeyField={handleRenderKeyField}
onRenderValueField={handleRenderValueField}
to
onRenderKeyField={handleKeyField}
onRenderValueField={handleValueField}
Is that ok or there is a better way to pass isEnabled
to handleRenderKeyField
and handleRenderValueField
without pass to AttributeEditor
?
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.
Thanks for the changes @ricardobessa1
)} | ||
/> | ||
); | ||
const QueryParamsEditor = ({ type, queryParams, isEnabled = true }) => { |
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.
Sorry, this might be causing you to flip-flip between implementations, but I think passing down isEnabled makes sense simply because I'd like to also see the addButton disabled (and maybe get @stevensideyliu opinion on if the removeButton should be disabled).
@mihirsoni I think it's still generic enough with that passed down, either the attribute editor is enabled or disabled and inputs/buttons need to reflect that.
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.
Sure makes sense. I think so in case of not enabled , we'll disable everything (All inputs , remove button and Add button)
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.
Thanks for feedback, as suggested I changed the code to pass down the isEnabled to AttributeEditor and disable Add and Remove buttons if isEnabled is false.
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.
Thanks for these changes @ricardobessa1 !
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. Thanks for changes @ricardobessa1
Related to issue #24
Description:
Added a flag isEnabled on QueryParamsEditor to control the disabled status from component.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.