Skip to content
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

Add Progress Component #121

Merged
merged 15 commits into from
Dec 7, 2015
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
"sinon-chai": "^2.8.0",
"through2": "^2.0.0",
"watch": "^0.16.0",
"webpack": "^1.12.2",
"webpack-dev-server": "^1.12.0"
"webpack": "1.12.1",
"webpack-dev-server": "1.10.1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
4 changes: 3 additions & 1 deletion src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import Segments from 'src/elements/Segment/Segments';

// Modules
import Checkbox from 'src/modules/Checkbox/Checkbox';
import Progress from 'src/modules/Progress/Progress';
import Modal from 'src/modules/Modal/Modal';
import ModalContent from 'src/modules/Modal/ModalContent';
import ModalFooter from 'src/modules/Modal/ModalFooter';
Expand Down Expand Up @@ -74,11 +75,12 @@ export default {

// Modules
Checkbox,
Dropdown,
Modal,
ModalContent,
ModalFooter,
ModalHeader,
Dropdown,
Progress,

// Views
Item,
Expand Down
95 changes: 95 additions & 0 deletions src/modules/Progress/Progress.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import React, {Component, PropTypes} from 'react';
import classNames from 'classnames';
import $ from 'jquery';
import META from 'src/utils/Meta';
import _ from 'lodash';

export default class Progress extends Component {
static propTypes = {
autoSuccess: PropTypes.bool,
children: PropTypes.node,
className: PropTypes.string,
label: PropTypes.oneOf(['ratio', 'percent']),
limitValues: PropTypes.bool,
onActive: PropTypes.func,
onChange: PropTypes.func,
onError: PropTypes.func,
onSuccess: PropTypes.func,
onWarning: PropTypes.func,
percent: PropTypes.number,
precision: PropTypes.number,
random: PropTypes.bool,
showActivity: PropTypes.bool,
total: PropTypes.bool,
value: PropTypes.bool,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These prop values were taken from semantic ui's progress docs:http://semantic-ui.com/modules/progress.html#/settings

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any of these props required? If so, can we add the .isRequired to help with prop management/usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically none are, you could possibly make the percent prop required, but it's not necessary.


componentDidMount() {
this.element = $(this.refs.element);
this.element.progress({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To cut down on SLOC it'd be nice to spread our progress props of interest here, e.g.

this.el.progress({...propsOfInterest})

Also, it might be better for UX do a document.createDocumentFragment in componentWillMount and then inject the DOM as soon as the root node of the component becomes available. But I haven't done much in the way of this kind of change so that's only conjecture at this point.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 to manual DOM manipulation in React, it will freak out if you touch the DOM. In this case, the progress plugin doesn't manipulate the DOM, only it's style. If we need to wrangle a jQuery plugin for DOM manipulation then use portals.

EDIT
Also, to create propsOfInterest we'd need to write the same amount of code to create an object consisting of the props we want in order to spread it. Prefer the current explicit list against a a new object that is used once.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eanplatter could you give this a try (as an example), it's highly expressive, doesn't require _, reduces typing and helps with minification (though as mentioned gzip will pretty much nullify the savings gained in the minified source over the wire):

componentDidMount() {
    let settings = [
      'autoSuccess',
      'children',
      'className',
      'label',
      'limitValues',
      'onActive',
      'onChange',
      'onError',
      'onSuccess',
      'onWarning',
      'percent',
      'precision',
      'random',
      'showActivity',
      'total',
      'value',
    ];
    settings = settings.map(setting => {
      const key = setting;
      return {[key]: this.props[key]};
    });

    this.element = $(this.refs.element);
    this.element.progress({...settings});
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return an array of objects with one key/value pair for each prop:

[
  {autoSuccess: <this.prop.autoSuccess>},
  {label: <this.prop.label>},
  ...etc
]

pseudo code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we omit children and classNames from the settings as well?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly what I wanted. Couldn't take it the full mile without the example, but we should be able to then iterate over the elements in the array and pass them into the progress method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming the same settings array, we can create the object like this:

  let settingsObj = {};
  settings.forEach(key => settingsObj[key] = this.props[key]);

  this.element.progress(settingsObj);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work?

  componentDidMount() {
    const whitelist = [
      'autoSuccess',
      'children',
      'className',
      'label',
      'limitValues',
      'onActive',
      'onChange',
      'onError',
      'onSuccess',
      'onWarning',
      'percent',
      'precision',
      'random',
      'showActivity',
      'total',
      'value',
    ];

    const settings = {};

    whitelist.map(key => {
      Object.assign(settings, {[key]: this.props[key]});
    });

    this.element = $(this.refs.element);
    this.element.progress({...settings});
  }

autoSuccess: this.props.autoSuccess,
label: this.props.label,
limitValues: this.props.limitValues,
onActive: this.props.onActive,
onChange: this.props.onChange,
onError: this.props.onError,
onSuccess: this.props.onSuccess,
onWarning: this.props.onWarning,
percent: this.props.percent,
precision: this.props.precision,
random: this.props.random,
showActivity: this.props.showActivity,
total: this.props.total,
value: this.props.value,
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are also taken from the stardust docs, and represent possible callbacks and plugin properties exposed to the user: http://semantic-ui.com/modules/progress.html#/usage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the settings accepted as props should be passed to the plugin here. Otherwise, we are defining props that are never used. For instance:

<Progress total={1} value={0.2} />

This should do this.element.progress({total: 1, value: 0.2}) on mount. Right now, the component accepts the props, validates them, then does nothing with them. So with the above example nothing will happen on mount.

If we instead pass all prop settings to the plugin, then it would mount and animate to 20% complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved all props to the this.element.progress method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing React children to a jQuery plugin are we ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and html classes.


static _meta = {
library: META.library.stardust,
name: 'Progress',
type: META.type.module,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta info for the component.


plugin() {
return this.element.progress(...arguments);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing the plugin property.


renderAttachedBar = () => {
return (
<div className='bar' />
);
};

renderStandardBar = () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've asked this before but I'm still a little fuzzy as to why we're not prefixing instance methods like this with _ (e.g. _renderStandardBar) to denote they are not apart of the api fingerprint for this component. /cc @levithomason

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Methods section of our fork of the Airbnb styleguide:

Do not use underscore prefix for internal methods of a react component.

Keep in mind these are not classes that users instantiate and use their methods as with a traditional library. JSX handle the class instantiation and interaction is done by props mostly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More in depth here: airbnb/javascript#490

const label = (
<div className='label'>
{this.props.children}
</div>
);

return (
<div>
<div className='bar'>
<div className='progress'/>
</div>
{this.props.children && label}
</div>
);
};

render() {
const classes = classNames(
'sd-progress',
'ui',
this.props.className,
'progress',
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configuring the classes.


const isAttached = _.contains(this.props.className, 'attached');
return (
<div {...this.props} className={classes}>
{isAttached ? this.renderAttachedBar() : this.renderStandardBar()}
</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The markup as shown in semantic ui's documentation: http://semantic-ui.com/modules/progress.html#/definition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like if there are children here they should be wrapped in a label div:

<div className='lablel'>{this.props.children}</div>

Make sure when there are no children there is not an empty label div present.

http://semantic-ui.com/modules/progress.html#label

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the attached class is present, there should only be a bar div without a progress child div:

<div className='ui segment'>
  <div className='ui top attached progress'>
    <div className='bar'></div>
  </div>
</div>

http://semantic-ui.com/modules/progress.html#attached

);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt like this was kind of messy, it was like, iteration number three though, so I decided to not waste anymore time and just try to get some feedback. Essentially just want to check if the attached class exists, and if so render the proper progress bar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In keeping with our style thus far (and removing the need for the bind operator) you can define the elements and conditions at the top of the render method. Then, use the conditions in the return to render the correct parts:

  render() {
    const isAttached = _.contains(this.props.className, 'attached');
    const attachedBar = <div className='bar' />;
    const label = (
      <div className='label'>
        {this.props.children}
      </div>
    );
    const standardBar = (
      <div>
        <div className='bar'>
          <div className='progress' />
        </div>
        {this.props.children && label}
      </div>
    );

    const classes = classNames(
      'sd-progress',
      'ui',
      this.props.className,
      'progress',
    );

    return (
      <div {...this.props} className={classes}>
        {isAttached ? attachedBar : standardBar}
      </div>
    );
  }

EDIT
Added support for conditional label div, if children are present.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually like how you broke those methods out though, then we aren't creating elements every render that we aren't using. We should really think about that moving forward.

1 change: 1 addition & 0 deletions test/mocks/SemanticjQuery-mock.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const jQueryPlugins = {
dropdown: sandbox.stub().returnsThis(),
modal: sandbox.stub().returnsThis(),
popup: sandbox.stub().returnsThis(),
progress: sandbox.stub().returnsThis(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Studding out since Progress is a stardust module.

transition: sandbox.stub().returnsThis(),
};

Expand Down
28 changes: 28 additions & 0 deletions test/specs/modules/Progress/Progress-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import {Progress} from 'stardust';

describe.only('Progress', () => {
it('should be able to receive children', () => {
render(
<Progress>
Child
</Progress>
).assertText('Child');
});

it('should create a div with the class of bar', () => {
render(<Progress />).findClass('bar');
});

it('should create two progress divs if un-attached', () => {
render(<Progress />)
.scryClass('progress')
.should.have.a.lengthOf(2);
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing that it has a child with class of bar


it('should not create extra progress div if attached', () => {
render(<Progress className='attached' />)
.scryClass('progress')
.should.have.a.lengthOf(1);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking that it has child with class of progress.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same testing notes