Bug 1840958: Move provision source to component #6278
Conversation
openshift-ci-robot
added
bugzilla/severity-medium
|
@yaacov: This pull request references Bugzilla bug 1840958, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
component/kubevirt
|
@suomiy please review |
| goToNetworkingStep={ | ||
| steps[VMWizardTab.NETWORKING]?.canJumpTo ? () => goToStep(VMWizardTab.NETWORKING) : null | ||
| } | ||
| getProvisionSourceAttribute={(attr: string) => |
There was a problem hiding this comment.
no need for such function as all the info is included in provisionSourceField and we can get it from there in the compoenent.
| provisionSourceField={this.getField(VMSettingsField.PROVISION_SOURCE_TYPE)} | ||
| onChange={this.props.onFieldChange} | ||
| goToStorageStep={ | ||
| steps[VMWizardTab.STORAGE]?.canJumpTo ? () => goToStep(VMWizardTab.STORAGE) : null |
There was a problem hiding this comment.
can we use useCallback so the React.memo can kick in?
There was a problem hiding this comment.
it's a class component can't use hooks here
There was a problem hiding this comment.
let's convert it to a functional component then, it should be pretty painless.
There was a problem hiding this comment.
🤯 it's a function now ...
| ProvisionSource.URL.toString(), | ||
| ProvisionSource.CONTAINER.toString(), | ||
| ProvisionSource.DISK.toString(), | ||
| ].includes(provisionSourceValue) && provisionSourceDiskHelpMsg} |
There was a problem hiding this comment.
it would be nice to have each one of these as a small component as well
yaacov
force-pushed
the
move-vm-wizard-source-to-component
branch
from
ec5e845
to
a214f2d
Compare
yaacov
force-pushed
the
move-vm-wizard-source-to-component
branch
from
a214f2d
to
f3e9f6a
Compare
| vmSettings, | ||
| onFieldChange, | ||
| }) => { | ||
| const getField = (key: VMSettingsField) => iGet(vmSettings, key); |
There was a problem hiding this comment.
these should useCallback as well
| </Button> | ||
| ); | ||
| const getStorageMsg = () => { | ||
| switch (provisionSourceValue) { |
There was a problem hiding this comment.
might be easier to call fromString once than toString 3times ..
| ); | ||
| }; | ||
|
|
||
| export const ProvisionSourceComponent: React.FC<ProvisionSourceComponentProps> = React.memo( |
|
@suomiy please re-review |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: suomiy, yaacov 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 |
|
@yaacov: All pull requests linked via external trackers have merged: openshift/console#6278, openshift/console#6277, openshift/console#6270. Bugzilla bug 1840958 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
move provision source to component per #6270 (comment)