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
fixes issue with form switch for name-value-editor on eventSources #4918
fixes issue with form switch for name-value-editor on eventSources #4918
Conversation
387091b
to
98a818f
Compare
@@ -62,7 +62,7 @@ const ApiServerSection: React.FC<ApiServerSectionProps> = ({ namespace }) => { | |||
return ( | |||
<FormSection title="ApiServerSource"> | |||
<FormGroup fieldId={fieldId} label="Resource" isRequired> | |||
<NameValueEditor | |||
<NameValueEditorComponent |
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.
Why not use AsynCOmponent
directly here instead of creating this new wrapper component?
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.
Yeah I added a separate one as it's used in two of the forms under sources so this way thought of using same wrapper both places.
import { AsyncComponent } from '@console/internal/components/utils/async'; | ||
|
||
const NameValueEditorComponent = (props) => ( | ||
<AsyncComponent |
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.
Better to directly use this in the form. Naming it NameValueEditorComponent
creates the confusion that this component does something different that the one from internal core package. I see that you've created this one because it is being used in two places. I won't mind having to use AsyncComponent twice but if you don't want that then maybe we should rename this to something else like AsyncNameValueEditor
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.
Makes sense , I'll update 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.
@rohitkrai03 have renamed to AsyncNameValueEditor
and moved to form as well, PTAL.
daa7f74
to
d49ea3b
Compare
/king bug |
d49ea3b
to
080facd
Compare
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: invincibleJai, rohitkrai03 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes:
https://issues.redhat.com/browse/ODC-3491
Analysis / Root cause:
UI breaks with on switch from sinkbinding to apiServerSource on click of add resource in nameValueEditor
Solution Description:
fixes issue with form switch for name-value-editor on eventSources form with Async
Browser conformance: