-
Notifications
You must be signed in to change notification settings - Fork 14
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: UI support for invokedByDefault transformers #128
feat: UI support for invokedByDefault transformers #128
Conversation
Thanks for making a pull request! 😃 |
Signed-off-by: Mehant Kammakomati <[email protected]>
8343351
to
c18dd86
Compare
src/app/qa/MultiLine.tsx
Outdated
@@ -20,7 +20,7 @@ import { QAContext } from './QAContext'; | |||
import { IQAComponentProps } from './QAWizard'; | |||
import { TextContent, TextArea } from '@patternfly/react-core'; | |||
|
|||
function Multiline(props: IQAComponentProps): JSX.Element { | |||
function MultiLine(props: IQAComponentProps): JSX.Element { |
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 is this change necessary?
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.
@HarikrishnanBalagopal @ashokponkumar
https://github.com/konveyor/move2kube-ui/pull/128/files#diff-9fbd7a7707d6fad02d98c83cc8c72a94821fa17a1983f54f1ef9c3da59bb1d25R113
Its MultiLineInput
in https://github.com/konveyor/move2kube/blob/main/types/qaengine/problem.go#L42 and Multiline
in UI due to which multi line type questions breaking the QA engine at UI.
Weed need to decide whether following MultiSelect
should we change this to MultiLine
or change it to MultiLineInput
which is there in move2kube code presently.
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.
In any case, this change is required for the QA engine not to break for multiline questions at UI
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.
This only changes the function name, it has zero impact in terms of what gets sent over the wire. Can you explain the bug you are seeing?
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.
Can you please check the completes changes? Function name is changed just for the styling. other changes fixes the issue. Thanks
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 see the issue https://github.com/konveyor/move2kube/blob/main/types/qaengine/problem.go#L42
currently has MultiLineInput
and the QA Wizard https://github.com/konveyor/move2kube-ui/blob/main/src/app/qa/QAWizard.tsx#L113 has Multiline
The only change required is to change https://github.com/konveyor/move2kube-ui/blob/main/src/app/qa/QAWizard.tsx#L113 to MultiLineInput
to match the CLI. It also makes it similar to Input
we already have. https://github.com/konveyor/move2kube-ui/blob/main/src/app/qa/QAWizard.tsx#L111
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.
But by the semantic meaning Input
can be anything.
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 notMultiSelectInput
🤔
Signed-off-by: Mehant Kammakomati <[email protected]>
Signed-off-by: Mehant Kammakomati <[email protected]>
Signed-off-by: Mehant Kammakomati <[email protected]>
Signed-off-by: Mehant Kammakomati <[email protected]>
@HarikrishnanBalagopal Made the changes, please review! |
fixes #129 |
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
Signed-off-by: Mehant Kammakomati [email protected]