[WIP] Implement Create VM Wizard component #4

rawagner · 2018-08-28T11:51:54Z

storybook is published here https://rawgit.com/rawagner/web-ui/create_VM_wizard_storybook/index.html

ohadlevy · 2018-08-28T11:57:57Z

thanks! do you mind publishing the storybook?

rawagner · 2018-08-28T12:05:20Z

@ohadlevy where should I publish it ? github pages of my forked repo ?

mareklibra · 2018-08-28T12:30:57Z

github pages of my forked repo

Seems to be best option. It will stay close to each PR.
And we can maintain storybook for master.

rawagner · 2018-08-28T12:45:29Z

@mareklibra OK, I will go with the github pages. Just be aware that github pages is using gh-pages or master branch. Which means you can have only one github page for repository. It will work for now, as I have only one PR but if I will have more open PRs I wont be able to host storybook for each of them.

ohadlevy · 2018-08-28T12:55:19Z

yeah, best is to use rawgit (so you can push multiple branches), have a look at pf-react for the npm task (i think its storybook-deployer)

…

On Tue, Aug 28, 2018 at 3:45 PM rawagner ***@***.***> wrote: @mareklibra <https://github.com/mareklibra> OK, I will go with the github pages. Just be aware that github pages is using gh-pages or master branch. Which means you can have only one github page for repository. It will work for now, as I have only one PR but if I will have more open PRs I wont be able to host storybook for each of them. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <kubevirt/web-ui#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOxzGMxoQzKzA0vAGmMtRi6NSCsK6iks5uVTtpgaJpZM4WPdLt> .

rawagner · 2018-08-28T13:20:20Z

@ohadlevy thanks! Storybook is published at https://rawgit.com/rawagner/web-ui/create_VM_wizard_storybook/index.html

ohadlevy · 2018-08-28T13:29:08Z

do we have a group of UX developers so we can ping them to look at the stroybook?

…

On Tue, Aug 28, 2018 at 4:20 PM rawagner ***@***.***> wrote: @ohadlevy <https://github.com/ohadlevy> thanks! Storybook is published at https://rawgit.com/rawagner/web-ui/create_VM_wizard_storybook/index.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <kubevirt/web-ui#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOxxXHlcl98q7JaNPcoaGnKFzS32THks5uVUOUgaJpZM4WPdLt> .

rawagner · 2018-08-28T13:36:44Z

@lizsurette

atiratree · 2018-08-28T12:00:47Z

src/components/wizards/create-vm/CreateVMWizard.js

@@ -0,0 +1,358 @@
+import React from 'react';


are we using a convention adding .jsx extension to files with jsx or using .js extensions everywhere?

@vojtechszocs @mareklibra any preference ?

I prefer just .js

I also prefer just .js for both React components and other JS code.

That's simply because the presence of JSX expressions doesn't make the particular file special, JSX is transformed into JS anyway 😃

+1 for just .js

atiratree · 2018-08-28T12:01:06Z

src/components/wizards/create-vm/CreateVMWizard.css

+
+.wizard-dropdown .caret {
+    float: right;
+}


missing newline + other files as well

added newline to all files

atiratree · 2018-08-28T12:42:45Z

src/components/wizards/new-vm/NewVMWizard.stories.js

+    ]
+
+    openWizard = () => {
+        this.setState({show: true});


NewVMWizard has to call this to change its own inner behaviour which is too complicated IMO. I would simply render or not render the component if it is openned or not. I think it is just enough to pass onClose callback to the NewVMWizard component which we can call from CreateVMWizard/ImportVMWizard. Then it is possible to simplify the code in NewVMWizard a bit.

Please take some inspiration from

https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-react/src/components/Wizard/Stories/WizardPatternExample.js

https://github.com/patternfly/patternfly-react/blob/master/packages/patternfly-react/src/components/Wizard/Stories/WizardPatternExampleStory.js

I guess what bothers me the most that there are 3 possible wizzards in the component and the prop callbacks are tangled to the component. Is it possible to make it somehow simpler?

I guess we could make the component lifecycle dependent only on the props or make it standalone.

I changed it to what @suomiy suggested. Just render or not render at all. Is is the preferred way ?

atiratree · 2018-08-28T12:53:41Z

src/components/wizards/new-vm/OperatingSystem.js

+export const os = [
+    {
+        name: 'Alpine Linux 3.5',
+        value: 'alpinelinux3.5'


maybe rename value to id?

renamed to id.

atiratree · 2018-08-28T13:19:11Z

src/components/wizards/create-vm/CreateVMWizard.js

+    }
+
+    closeWizard = () => {
+      this.setState(defaultState);


I think there will not be a need for this if we use the no component approach discussed in NewVMWizard.stories.js + you can return the defaultState to constructor

atiratree · 2018-08-28T14:02:16Z

src/components/wizards/create-vm/CreateVMWizard.js

+            }
+            let child = null;
+            const valid = _.get(this.state.basicSettings[key], 'valid', null);
+            if (this.basicSettings[key].type === 'text') {


please consider using switch

atiratree · 2018-08-28T14:03:52Z

src/components/wizards/create-vm/CreateVMWizard.js

+        render: () => <Form horizontal>
+          {Object.keys(this.basicSettings).map(key => {
+            if (this.basicSettings[key].isVisible && this.basicSettings[key].isVisible(this.state.basicSettings) === false){
+              return null;


can be undefined,
let child; also

atiratree · 2018-08-28T14:04:57Z

src/components/wizards/create-vm/CreateVMWizard.js

+              </ButtonGroup>;
+            }
+            if (this.basicSettings[key].type === 'checkbox') {
+              child = <Checkbox checked={_.get(this.state.basicSettings[key], 'value', false)} onChange={event => this.onFormChange(event.target.checked, key)}>{this.basicSettings[key].title}</Checkbox>;


please put props on new lines for better readability

atiratree · 2018-08-28T14:07:45Z

src/components/wizards/create-vm/CreateVMWizard.js

+            if (this.basicSettings[key].type === 'checkbox') {
+              child = <Checkbox checked={_.get(this.state.basicSettings[key], 'value', false)} onChange={event => this.onFormChange(event.target.checked, key)}>{this.basicSettings[key].title}</Checkbox>;
+            }
+            return <FormGroup key={key} validationState={valid ? 'error' : null}>


please use brackets () and put FormGroup on the newline. IMO it is more readable because of the same spacing

applies also elsewhere

done, hopefully everywhere

Yes, this is also the syntax form preferred by patternfly-react project.

atiratree · 2018-08-28T14:11:34Z

src/components/wizards/create-vm/CreateVMWizard.js

+      }
+    ]
+
+    createVM = (basicSettings, network, storage) => {


I would rather see this in another file and give it a data in stable format. Maybe some file like requests.js ? Then it would create final data structure vm.

OK, I moved it to k8s/request.js

vojtechszocs

Looks OK in general.

Code style should be close to patternfly-react.

Components should be isolated in terms of CSS if we really care about true reusability (i.e. use CSS modules). We can improve that later on.

vojtechszocs · 2018-08-28T13:49:01Z

src/components/wizards/create-vm/CreateVMWizard.css

@@ -0,0 +1,7 @@
+.wizard-dropdown {


I'd suggest to follow CamelCase convention like CreateVmWizard.css etc.

Also, I'd suggest using code style with 2 spaces used for indentation, both in CSS and JS.

vojtechszocs · 2018-08-28T13:51:34Z

src/components/wizards/create-vm/CreateVMWizard.js

@@ -0,0 +1,366 @@
+import React from 'react';
+import { Form, FormControl, Col, ButtonGroup, DropdownButton, MenuItem, Checkbox, FormGroup, ControlLabel, HelpBlock, Wizard } from 'patternfly-react';


In future, very long imports (past X chars) should be written as

import { a, b, c } from 'foo'

just like in other projects.

vojtechszocs · 2018-08-28T13:57:10Z

src/components/wizards/create-vm/CreateVMWizard.js

+
+    onFormChange = (newValue, target) => {
+      let valid = null;
+      if (this.basicSettings[target].required && newValue.trim().length === 0){


We are using this.basicSettings because we don't need component re-render when it changes, right?

What's the intended relation between this.basicSettings and this.state.basicSettings ?

this.basicSettings represents fields of form (type, label, validation). this.state.basicSettings represents actual values that are changing based on user input (value, validation message). Maybe I should rename one of them to avoid confusion

I see, I'd rename them to better reflect their purpose, i.e. this.basicStepFields and this.state.basicStepValues or similar.

I'd use the word "step" since it's the grouping concept that puts multiple form fields together, from visual/user perspective.

vojtechszocs · 2018-08-28T13:59:44Z

src/components/wizards/create-vm/CreateVMWizard.js

+        ...this.state.basicSettings,
+        [target]: {
+          value: newValue,
+          valid: valid


You can use ES6 object property shorthand syntax - valid instead of valid: valid.

vojtechszocs · 2018-08-28T14:00:44Z

src/components/wizards/create-vm/CreateVMWizard.js

+    }
+
+    onFormChange = (newValue, target) => {
+      let valid = null;


To avoid confusion with wizardValid being a boolean, can we call this validMsg or similar?

renamed to validMsg

vojtechszocs · 2018-08-28T14:24:52Z

src/components/wizards/create-vm/CreateVMWizard.js

+
+    closeWizard = () => {
+      this.setState(defaultState);
+      this.props.closeWizard();


If closeWizard is meant to be a callback prop, please call it onClose instead.

Also, patternfly-react Wizard already provides onExited callback prop that executes when the modal finishes its fade-out transition, I'd suggest using that instead.

vojtechszocs · 2018-08-28T14:28:02Z

src/components/wizards/create-vm/CreateVMWizard.js

+        show={this.props.show}
+        onHide={this.closeWizard}
+        onClose={this.closeWizard}
+        steps={this.wizardStepsNewVM}


steps is where the vast majority of wizard UI & logic lives.

I think that in future, we should find a better representation, I recall talking with patternfly-react folks about allowing steps to be represented as React components, instead of plain JS array.

vojtechszocs · 2018-08-28T14:33:29Z

src/components/wizards/create-vm/CreateVMWizard.js

+    render() {
+      return <Wizard.Pattern
+        show={this.props.show}
+        onHide={this.closeWizard}


Wizard's onClose is triggered when you click the "X" button in top right corner. WizardPattern implements it for us, but doesn't expose it as a prop, so onClose should be removed here.

As for onHide, this is a function that WizardPattern calls like so:

onHide(onFinalStep) // true or false, whether we are on the final step

TL;DR I'd just keep the onHide and remove onClose.

onClose removed

vojtechszocs · 2018-08-28T14:35:47Z

src/components/wizards/create-vm/CreateVMWizard.js

+        activeStepIndex={this.state.activeStepIndex}
+        onStepChanged={index => this.onStepChanged(index)}
+        nextStepDisabled={!this.state.wizardValid}
+        nextText={this.state.activeStepIndex === 2 ? 'Create Virtual Machine':'Next'} />;


As for JSX expressions, please use one of these forms (in-line with patternfly-react):

<Example foo={bar} /> // lots of props <Example foo={bar} /> <Example foo={bar}> <AnotherExample /> </Example> // lots of props <Example foo={bar} > <AnotherExample /> </Example>

vojtechszocs · 2018-08-28T14:40:30Z

src/components/wizards/create-vm/StorageTable.js

+
+export class StorageTable extends React.Component {
+
+    state = {


You can use this form in CreateVmWizard component too.

Let's be consistent with how we initialize component's state, using ES6 class fields sounds good to me.

Ok, class fields it is.

lizsurette · 2018-08-29T13:24:14Z

src/components/wizards/create-vm/CreateVmWizard.js

+        type: 'checkbox'
+      },
+      createTemplate:{
+        title: 'Create new teplate from configuration',


typo on 'template'

atiratree · 2018-08-29T13:18:53Z

src/components/wizards/new-vm/NewVmWizard.stories.js

+      <React.Fragment>
+        <Button onClick={this.openWizard}>Open New VM Wizard</Button>
+          {this.state.show ? <NewVmWizard
+            closeWizard={this.closeWizard}


I think onClose would be more fitting

I agree, in React world, onSomething is a standard convention for callback props.

renamed to onHide

atiratree · 2018-08-29T13:20:06Z

src/components/wizards/new-vm/NewVmWizard.js

+              <Wizard.Row>
+                <Wizard.Main>
+                  <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content">
+                    <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}>


You could make a component out of this

there is a pf-react Button Component?

there is, but I don't think it is needed so much. I just meant lines 33-40 altogether .

I will use Button from pf-react and also extract the whole 'button with icon and label' to separate component

Yes, let's reuse patternfly-react's Button if possible.

Moved to separate component ButtonWithIcon which is using Button, Icon and Col pf-react components

atiratree · 2018-08-29T13:21:18Z

src/components/wizards/create-vm/CreateVmWizard.js

+        values: this.props.flavors.concat([{name:'Custom'}]),
+        required: true
+      },
+      memory:{


please better validation for memory/cpu/URL

atiratree · 2018-08-29T13:23:11Z

src/utils/forms/FormFactory.js

+          </Col>
+        </FormGroup>);
+  })
+}


newline - better to setup an IDE to do it for you ..

atiratree · 2018-08-29T13:23:34Z

src/utils/forms/FormFactory.js

+export const FormFactory = (props) => {
+  return Object.keys(props.fields).map(key => {
+    if (props.fields[key].isVisible && props.fields[key].isVisible(props.fieldsValues) === false){
+      return undefined;


just return; is enough

this results in eslint warning. Anyways I avoided it by filtering the fields which are not visible and than map only the visible fields.

atiratree · 2018-08-29T13:25:28Z

src/k8s/request.js

+  if(basicSettings.description){
+    vm.metadata.labels.description = basicSettings.description;
+  }
+  addFlavor(vm, basicSettings);


I think few newlines here and there add to the readability, but that is just my opinion.

added a few new lines here and there :)

atiratree · 2018-08-29T13:25:46Z

src/constants/index.js

+export const API_VERSION='kubevirt.io/v1alpha2'
+export const VM_KIND='VirtualMachine'
+export const OS_LABEL='kubevirt.io/os'
+export const FLAVOR_LABEL='kubevirt.io/flavor'


atiratree · 2018-08-29T13:25:56Z

src/components/wizards/new-vm/NewVmWizard.js

+        </React.Fragment>
+      );
+    }
+}


Are we using React Proptypes in this project or not ?

we should, 👍

Of course we should use them 😃 maybe not right now, since we're prototyping our first complex wizard component, but we should use them at some point.

ohadlevy · 2018-08-29T13:27:48Z

src/components/wizards/create-vm/CreateVmWizard.js

+  Wizard
+} from 'patternfly-react';
+import './CreateVmWizard.css';
+import * as _ from 'lodash';


please don't import the entire lodash, instead do:

import get from 'lodash/get'

IIRC there are webpack plugins to address these kinds of optimization issues, I will look into that later on.

changed all import * as _ from 'lodash to more precise ones (get/has etc)

ohadlevy · 2018-08-29T13:28:31Z

src/components/wizards/create-vm/CreateVmWizard.js

+
+export class CreateVmWizard extends React.Component {
+
+    state = {


should this be:

const state = { ... }

no, it should not. It is a class property which is not 'prefixed' with const

ohadlevy · 2018-08-29T13:29:21Z

src/components/wizards/create-vm/CreateVmWizard.js

+    onFormChange = (newValue, target) => {
+      let validMsg = null;
+      if (this.formFields[target].required && newValue.trim().length === 0){
+        validMsg = `${this.formFields[target].title} is required`;


this is not i18n friendly, @vojtechszocs add to the list of project infra?

overall, i feel this should probably live outside of the component in some i18n or forms related code?

this is not i18n friendly, @vojtechszocs add to the list of project infra?

Added that to the list of infra tasks.

We should use the standard ICU message format. In ovirt-engine-ui-extensions project, we're using intl-messageformat - every localized message is a function that accepts zero or more parameters, returning a localized string.

overall, i feel this should probably live outside of the component in some i18n or forms related code?

Yes, i18n is another concern and should be put into its dedicated module.

I will wait for infra updates then

ohadlevy · 2018-08-29T13:32:34Z

src/components/wizards/create-vm/CreateVmWizard.js

+      const requiredKeys = Object.keys(this.formFields).filter(key => this.isFieldRequired(key, values));
+      const requiredKeysInValues = Object.keys(values).filter(key => this.isFieldRequired(key, values));
+      if(requiredKeys.length !== requiredKeysInValues.length){
+        wizardValid = false;


i wonder if using something like finaly-form or formik would save us some effort here.

Thanks @ohadlevy for the links!

finaly-form is React agnostic, implements observer pattern to subscribe to form/field changes and allows their updates.

formik is React specific, and from quickly looking at their examples, is geared towards HTML <form> handling, i.e. the case when you use DOM form/field elements directly.

ohadlevy · 2018-08-29T14:09:27Z

src/components/wizards/new-vm/NewVmWizard.js

+              <Wizard.Row>
+                <Wizard.Main>
+                  <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content">
+                    <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}>


there is a pf-react Button Component?

ohadlevy · 2018-08-29T14:09:44Z

src/components/wizards/new-vm/NewVmWizard.js

+                <Wizard.Main>
+                  <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content">
+                    <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}>
+                      <div className="col-md-12">


use Col component ?

Yup, we should use patternfly-react's Grid, Row and Col components instead of plain HTML/CSS markup.

Also, at some point, we'll transition to patternfly-react v4 which will implement its own grid system.

pf-react Button, Col and Icon used instead

ohadlevy · 2018-08-29T14:10:12Z

src/components/wizards/new-vm/NewVmWizard.js

+                  <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content">
+                    <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}>
+                      <div className="col-md-12">
+                        <span className="pficon-virtual-machine fa-5x" />


use Icon component?

@vojtechszocs @mareklibra do you think it is better to use components or simple classes ?

ohadlevy · 2018-08-29T14:10:29Z

src/components/wizards/new-vm/NewVmWizard.js

+        </React.Fragment>
+      );
+    }
+}


we should, 👍

ohadlevy · 2018-08-29T14:10:49Z

src/components/wizards/new-vm/NewVmWizard.stories.js

+    show: false
+  }
+
+  storageClasses = [


maybe move to a fixture files? could be reused in tests?

sounds good, TODO

lizsurette · 2018-08-29T14:13:48Z

@rawagner

Agreed with others, this is looking great! A few overall comments:

When the user selects a provision source, what do you think about removing the indentation for any fields that are added? I think it will read better if they are inline. (Similar to how the fields show when the Enable cloud-init box is checked.
Would it be possible to split the OS field into Operating System and Version? I got some feedback that this list could get super long if we leave it as one. Of course this would mean that depending on which OS is selected, the versions would need to be updated.
Although it might be annoying to need two different styles, could we tighten up the padding between the check box options at the bottom of the Basic Settings steps? Maybe just allow them to live within one form-group if possible?

ohadlevy · 2018-08-29T14:27:23Z

note its used below, so I guess it should be used consistently.

…

On Wed, Aug 29, 2018 at 5:26 PM suomiy ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/wizards/new-vm/NewVmWizard.js <kubevirt/web-ui#4 (comment)>: > + render() { + let wizard; + if(this.state.createVM) { + wizard = <CreateVmWizard onHide={this.props.closeWizard} {...this.props}/> + } else if(this.state.importVM) { + wizard = <ImportVmWizard onHide={this.props.closeWizard} {...this.props}/> + } + return ( + <React.Fragment> + <Wizard show onHide={this.props.closeWizard}> + <Wizard.Header title="Create Virtual Machine" onClose={this.props.closeWizard} /> + <Wizard.Body> + <Wizard.Row> + <Wizard.Main> + <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content"> + <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}> there is, but I don't think it is needed so much. I just meant lines 33-40 altogether . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <kubevirt/web-ui#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx-mnjdsstQsqWmXvrQtq6FF-8W9kks5uVqSTgaJpZM4WPdLt> .

ohadlevy · 2018-08-29T14:38:52Z

there is, but i still think its a better practice to get used to instead of importing all of it.

…

On Wed, Aug 29, 2018 at 5:36 PM Vojtech Szocs ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/wizards/create-vm/CreateVmWizard.js <kubevirt/web-ui#4 (comment)>: > @@ -0,0 +1,228 @@ +import React from 'react'; +import { + Form, + Wizard +} from 'patternfly-react'; +import './CreateVmWizard.css'; +import * as _ from 'lodash'; IIRC there are webpack plugins to address these kinds of optimization issues, I will look into that later on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <kubevirt/web-ui#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx-279HZf9SAL-aDEoAqzy9O93KVaks5uVqbygaJpZM4WPdLt> .

atiratree · 2018-08-29T14:46:35Z

Would it be possible to split the OS field into Operating System and Version? I got some feedback that this list could get super long if we leave it as one. Of course this would mean that depending on which OS is selected, the versions would need to be updated.

It makes sense, but I would leave it until we know that we get this kind of information from the API - which will be preset os labels.

ohadlevy · 2018-08-29T15:11:52Z

not before tectonic migration please..

…

On Wed, Aug 29, 2018 at 5:49 PM Vojtech Szocs ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/wizards/new-vm/NewVmWizard.js <kubevirt/web-ui#4 (comment)>: > + let wizard; + if(this.state.createVM) { + wizard = <CreateVmWizard onHide={this.props.closeWizard} {...this.props}/> + } else if(this.state.importVM) { + wizard = <ImportVmWizard onHide={this.props.closeWizard} {...this.props}/> + } + return ( + <React.Fragment> + <Wizard show onHide={this.props.closeWizard}> + <Wizard.Header title="Create Virtual Machine" onClose={this.props.closeWizard} /> + <Wizard.Body> + <Wizard.Row> + <Wizard.Main> + <Wizard.Contents stepIndex={0} activeStepIndex={0} className="wizard-content"> + <button className="btn btn-link wizard-button " onClick={this.openCreateVmWizard}> + <div className="col-md-12"> Yup, we should use patternfly-react's Grid, Row and Col components instead of plain HTML/CSS markup. Also, at some point, we'll transition to patternfly-react v4 which will implement its own grid system. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <kubevirt/web-ui#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOxx4KVaujoCKgD17iLtJPqgxAdX1iks5uVqnlgaJpZM4WPdLt> .

rawagner · 2018-08-30T09:05:28Z

@lizsurette

When the user selects a provision source, what do you think about removing the indentation for any fields that are added? I think it will read better if they are inline. (Similar to how the fields show when the Enable cloud-init box is checked.

No problem, I saw it in earlier version of Create VM design. It is now removed, less work for me :)

Would it be possible to split the OS field into Operating System and Version? I got some feedback that this list could get super long if we leave it as one. Of course this would mean that depending on which OS is selected, the versions would need to be updated.

as @suomiy said, lets wait for API updates, we will change it then. I agree that if we should list all possible values into one dropdrown, it would be a huge one..

Although it might be annoying to need two different styles, could we tighten up the padding between the check box options at the bottom of the Basic Settings steps? Maybe just allow them to live within one form-group if possible?

I will, TODO

rawagner · 2018-09-04T11:47:05Z

Added some enzyme tests.

@ohadlevy any suggestions on what should we use for code coverage metrics ?

ohadlevy · 2018-09-04T12:33:24Z

@rawagner kubevirt/web-ui#16

coveralls · 2018-09-04T12:41:15Z

Pull Request Test Coverage Report for Build 40

146 of 161 (90.68%) changed or added relevant lines in 15 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+67.8%) to 85.95%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/buttons/ButtonWithIcon.js	2	3	66.67%
src/components/forms/Dropdown.js	4	5	80.0%
src/k8s/request.js	54	57	94.74%
src/components/wizards/import-vm/ImportVmWizard.js	1	5	20.0%
src/components/wizards/create-vm/CreateVmWizard.js	32	38	84.21%

Totals
Change from base Build 37:	67.8%
Covered Lines:	148
Relevant Lines:	168

💛 - Coveralls

ohadlevy · 2018-09-04T12:59:22Z

src/components/forms/FormControls.js

+  onChange: PropTypes.func.isRequired
+};
+
+export const DropdownControl = ({ fieldKey, value, choices, onChange }) => (


I personally prefer a component in a file, it makes it easier to find, maybe just change them to forms/textArea, forms/checkBox etc, would be trivial to find ?

ohadlevy · 2018-09-04T13:15:30Z

src/components/forms/__tests__/__snapshots__/FormFactory.test.js.snap

@@ -0,0 +1,186 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP


I've found that snapshots over 50 lines is usually ignored, if some of the internal snaps are already part of other snapshots maybe they could be avoided?

reduced to < 50 lines.NewVmWizard.test.js.snap still has 66 lines but I do not see a way to reduce it.

I've found that snapshots over 50 lines is usually ignored, if some of the internal snaps are already part of other snapshots maybe they could be avoided?

This is true for snapshots and other files too. If it looks big and complex, people are less inclined to review and maintain it, until it breaks the build or application itself.

This has already been discussed on patternfly-react project, see my comment on Enzyme & snapshot testing.

In particular:

as @dmiller9911 pointed out, snapshots shouldn't include fully-resolved HTML tree, otherwise you'll end up with big snapshots and it's hard to differentiate the actual component's HTML bits

reduced to < 50 lines. NewVmWizard.test.js.snap still has 66 lines but I do not see a way to reduce it.

Snapshot of NewVmWizard component should include virtual DOM of the component itself. It should not include fully-resolved HTML tree.

ohadlevy · 2018-09-04T14:32:10Z

src/components/forms/controls/index.js

+import { DropdownControl } from './DropdownControl';
+import { CheckboxControl } from './CheckboxControl';
+
+export { TextAreaControl, TextControl, DropdownControl, CheckboxControl };


you can also use the following syntax:

export { TextAreaControl } from './TextAreaControl';

ohadlevy · 2018-09-04T14:33:05Z

src/components/wizards/create-vm/CreateVmWizard.js

+      validMsg = this.basicFormFields[target].validate(newValue);
+    }
+    if (this.basicFormFields[target].required && newValue.trim().length === 0) {
+      validMsg = 'is required';


I wonder if we should do something with the strings to make it easier later on to i18n.

maybe we could address that in followup patch ? :) Im not sure what solution for i18n we will want to use. It would be nice to keep aligned with openshift console but they do not have any i18n yet.

The openshift/console project (frontend part) currently doesn't seem to use any i18n mechanism, there are English strings hardcoded in their components.

I'd like to avoid adding complexity until we really need it. For now, I would focus on writing simple, reusable components (and break down bigger components into smaller bits for that matter) along with associated .test.js and .stories.js files.

rawagner · 2018-09-06T07:41:41Z

Latest PR updates code coverage configuration to exclude *.stories.js files

ohadlevy · 2018-09-06T07:50:33Z

package.json

@@ -79,7 +82,8 @@
  "jest": {
    "collectCoverage": true,
    "collectCoverageFrom": [
-      "src/**/*.{js,jsx,mjs}"
+      "src/**/*.{js,jsx,mjs}",
+      "!src/**/*.stories.js"


👍 maybe we can just PR this right away?

ohadlevy · 2018-09-06T12:44:01Z

@rawagner thanks, it looks like there is a snapshot out of date + css linting errors in the tests.

vojtechszocs · 2018-09-07T14:00:12Z

I've got some infra changes coming up that will help formalize the project structure and conventions, including tools for validation and consistent usage of other tools (like using one Babel config for webpack, Jest and Storybook).

atiratree · 2018-09-12T17:48:10Z

I refactored FormFactory.js, FormControls and CreateVmWizard.js in the followup. So if somebody notices something in there, we can discuss it in #10 :) and proceed with merging this PR.

coveralls · 2018-09-18T08:04:47Z

Pull Request Test Coverage Report for Build 74

146 of 161 (90.68%) changed or added relevant lines in 15 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-10.8%) to 89.224%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/buttons/ButtonWithIcon.js	2	3	66.67%
src/components/forms/Dropdown.js	4	5	80.0%
src/k8s/request.js	54	57	94.74%
src/components/wizards/import-vm/ImportVmWizard.js	1	5	20.0%
src/components/wizards/create-vm/CreateVmWizard.js	32	38	84.21%

Totals
Change from base Build 72:	-10.8%
Covered Lines:	147
Relevant Lines:	162

💛 - Coveralls

rawagner · 2018-09-25T11:07:34Z

deprecated by #27

rawagner force-pushed the create_VM_wizard branch from 1d5ea60 to d261857 Compare August 28, 2018 12:47

atiratree reviewed Aug 28, 2018

View reviewed changes

vojtechszocs reviewed Aug 28, 2018

View reviewed changes

rawagner force-pushed the create_VM_wizard branch 8 times, most recently from 3d5a0ab to 45b6e0e Compare August 29, 2018 12:16

lizsurette reviewed Aug 29, 2018

View reviewed changes

atiratree reviewed Aug 29, 2018

View reviewed changes

ohadlevy reviewed Aug 29, 2018

View reviewed changes

rawagner force-pushed the create_VM_wizard branch from 45b6e0e to cfb352b Compare August 30, 2018 08:51

rawagner force-pushed the create_VM_wizard branch from cfb352b to 38e09a0 Compare August 30, 2018 09:10

rawagner force-pushed the create_VM_wizard branch from d47120c to bb07399 Compare September 4, 2018 11:35

rawagner force-pushed the create_VM_wizard branch from bb07399 to 959ac45 Compare September 4, 2018 12:37

ohadlevy reviewed Sep 4, 2018

View reviewed changes

rawagner force-pushed the create_VM_wizard branch 4 times, most recently from a7e08a8 to a8ce6a8 Compare September 4, 2018 14:15

ohadlevy reviewed Sep 4, 2018

View reviewed changes

rawagner force-pushed the create_VM_wizard branch 4 times, most recently from 2fb0ba4 to 190a137 Compare September 6, 2018 07:41

ohadlevy reviewed Sep 6, 2018

View reviewed changes

rawagner force-pushed the create_VM_wizard branch from 190a137 to 8256c38 Compare September 6, 2018 11:54

rawagner force-pushed the create_VM_wizard branch from 8256c38 to fbf1b30 Compare September 6, 2018 14:42

Implement Create VM Wizard component

527939a

rawagner force-pushed the create_VM_wizard branch from fbf1b30 to 527939a Compare September 18, 2018 07:59

rawagner closed this Sep 25, 2018

rawagner deleted the create_VM_wizard branch September 27, 2018 13:40

		@@ -0,0 +1,366 @@
		import React from 'react';
		import { Form, FormControl, Col, ButtonGroup, DropdownButton, MenuItem, Checkbox, FormGroup, ControlLabel, HelpBlock, Wizard } from 'patternfly-react';


		export class StorageTable extends React.Component {

		state = {


		export class CreateVmWizard extends React.Component {

		state = {

[WIP] Implement Create VM Wizard component #4

[WIP] Implement Create VM Wizard component #4

Conversation

rawagner commented Aug 28, 2018 • edited Loading

ohadlevy commented Aug 28, 2018

rawagner commented Aug 28, 2018

mareklibra commented Aug 28, 2018

rawagner commented Aug 28, 2018

ohadlevy commented Aug 28, 2018 via email

rawagner commented Aug 28, 2018

ohadlevy commented Aug 28, 2018 via email

rawagner commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rawagner commented Aug 28, 2018 •

edited

Loading