-
Notifications
You must be signed in to change notification settings - Fork 30
Conversation
Pull Request Test Coverage Report for Build 114
💛 - Coveralls |
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, clean-up required.
I like the FormFactrory
.
A few comments inscribed.
@@ -0,0 +1,302 @@ | |||
import React from 'react'; |
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.
Please rename folder to follow component name (CamelCase), so .../wizards/CreateVmWizard/...
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.
fixed for all components
openCreateVmWizard = () => this.setState({ createVM: true }); | ||
|
||
render() { | ||
let wizard; |
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 be:
const wizard = this.state.createVM && (...)
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.
fixed
templates: this.props.templates | ||
}; | ||
|
||
componentDidUpdate({ namespaces, templates }) { |
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.
Please use getDerivedStateFromProps()
instead
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.
replaced componentDidUpdate with getDerivedStateFromProps
src/components/forms/FormFactory.js
Outdated
import { get, has } from 'lodash'; | ||
import { TextArea, Dropdown, Checkbox, Text } from '.'; | ||
|
||
export const FormFactory = ({ fields, fieldsValues, onFormChange }) => { |
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.
Nice :-)
src/components/forms/FormFactory.js
Outdated
|
||
export const FormFactory = ({ fields, fieldsValues, onFormChange }) => { | ||
const formGroups = Object.keys(fields) | ||
.filter(key => !(fields[key].isVisible && fields[key].isVisible(fieldsValues) === 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.
nitpick to be generic:
change fields[key].isVisible(fieldsValues)
to !!fields[key].isVisible(fieldsValues)
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.
fixed
src/components/forms/FormFactory.js
Outdated
/> | ||
); | ||
break; | ||
default: |
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.
nitpick to be genric: introduce specific type
to be same as this default
, like:
case 'text':
default:
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.
fixed
} | ||
} | ||
})); | ||
this.validateWizard(basicVmSettings); |
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.
good idea here ...
} from '../../../constants'; | ||
import { getTemplatesWithLabels, getTemplatesLabelValues } from '../../../utils/template'; | ||
|
||
/* eslint react/no-did-update-set-state: 0 */ |
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 will probably not needed with the changes bellow
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.
removed
return false; | ||
}; | ||
|
||
getFlavorLabel = () => { |
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.
For a follow-up: improve behavior from user-perspective, so the user is still able to select any OS, na matter what pair-option (flavor,workload) has selected.
Just ideas:
- disable unselected values (to denote "There's something more")
- add sort of "reset form" button
- add generic value to the dropdows to reset selection
- etc.
@@ -0,0 +1,98 @@ | |||
export const fedora28 = { |
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.
please rename folte templates
to mock_templates
to avoid confusion.
These templates are expected to be loaded from API once implemented within kubevirt Operator (or so)
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.
renamed to mock_templates
@rawagner any difference from what you had shared in Storybook? Would you mind adding screenshots, if so? |
@lizsurette There is basically no visual difference, just removed 'Create Template from VM' checkbox as we do not have that functionality implemented yet. I will share Cosmos (alternative to Storybook) soon :) |
a5ceb99
to
57e95ca
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, just minor comments
package.json
Outdated
@@ -24,7 +24,7 @@ | |||
"test:watch": "yarn test --watch", | |||
"prebuild": "yarn test --ci && shx rm -rf dist", | |||
"build": "yarn build:js && yarn build:sass", | |||
"build:js": "babel src --config-file ./config/babel.config.js --out-dir dist/js --only 'src/components/**/*.js,src/index.js' --ignore '**/tests/**,**/fixtures/**'", | |||
"build:js": "babel src --config-file ./config/babel.config.js --out-dir dist/js --only 'src/components/**/*.js,src/constants/**/*.js,src/k8s/**/*.js,src/models/**/*.js,src/utils/**/*.js,src/index.js' --ignore '**/tests/**,**/fixtures/**'", |
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't we simplify that to just src/**/*.js
?
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.
added src/**/*.js
and ignore now includes jest
and cosmos
folders as those releate to tests which we are not shipping
}; | ||
|
||
FormFactory.propTypes = { | ||
fields: PropTypes.object.isRequired, |
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.
For a follow-up: we should be more specific here
|
||
FormFactory.propTypes = { | ||
fields: PropTypes.object.isRequired, | ||
fieldsValues: PropTypes.object.isRequired, |
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.
likewise
}; | ||
|
||
getFlavors = () => { | ||
let flavors = []; |
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.
No need to initialize here, can be const flavors
later.
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.
fixed
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
@lizsurette I'm working on deploying Cosmos (Storybook alternative) static application upon PR merge so we have an up-to-date interactive showcase of our KubeVirt UI components. |
@vojtechszocs perfect, thank you! |
No description provided.