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
Merged
Binary file added .DS_Store
Binary file not shown.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ bower_components/
testBundle.js
docs/build
docgenInfo.json
.idea/
Copy link
Member

Choose a reason for hiding this comment

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

Use global gitignore for the .idea/directory. This way not all projects have to have this entry, also not all developers will use a JetBrains product so this ignore only applies to a subset of users.

Copy link
Member

Choose a reason for hiding this comment

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

Also, add .DS_Store to your global ignore. Then remove it from this PR.

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
65 changes: 65 additions & 0 deletions src/modules/Progress/Progress.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import React, {Component, PropTypes} from 'react';
import classNames from 'classnames';
import $ from 'jquery';
import META from 'src/utils/Meta';

export default class Progress extends Component {
static propTypes = {
autoSuccess: PropTypes.bool,
children: PropTypes.node,
className: PropTypes.string,
label: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

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

label only accepts two values, this should be PropType.oneOf('percent', 'ratio'). Unless this prop is referring to a progress element label, and not the plugin property label. In that case, there is a name conflict and we should fix it.

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

label Can be set to either to display progress as percent or ratio. Matches up to corresponding text template with the same name.

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});
  }

onActive: this.props.onActive,
onChange: this.props.onChange,
onError: this.props.onError,
onSuccess: this.props.onSuccess,
onWarning: this.props.onWarning,
percent: this.props.percent,
});
}
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.


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.


return (
<div {...this.props} className={classes}>
<div className='bar'>
<div className='progress'/>
</div>
{this.props.children}
</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
18 changes: 18 additions & 0 deletions test/specs/modules/Progress/Progress-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react';
import {Progress} from 'stardust';

describe('Progress', () => {
it('should be able to receive children', () => {
render(
<Progress>
<div className='new-child' />
</Progress>
).scryClass('new-child');
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 picks up it's children.

Copy link
Member

Choose a reason for hiding this comment

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

scry methods return arrays but do not throw if there are 0 results. So, this test has no assertion it will always pass. You can change it to findClass which will throw an error if it finds 0, or more than 1. Or, we've added this assertText which does a simple assertion that text is present (text is still a child):

render(<Progress>child</Progress>)
  .assertText('child');

});
it('should create a div with the class of bar', () => {
render(<Progress />).scryClass('bar');
});
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 create a div with the class of progress', () => {
render(<Progress />).scryClass('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.

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