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
Binary file added .DS_Store
Binary file not shown.
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
97 changes: 97 additions & 0 deletions src/modules/Progress/Progress.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
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});
  }

autoSuccess: this.props.autoSuccess,
children: this.props.children,
className: this.props.className,
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' />
);
}
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 for an attached 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.

Single line of JSX, return <div...


renderStandardBar() {
return (
<div>
<div className='bar'>
<div className='progress'/>
</div>
<div className='label'>
{this.props.children}
</div>
</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 for a normal progress bar.


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.


let content = ::this.renderStandardBar();
Copy link

Choose a reason for hiding this comment

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

Though this app is currently set-up for stage: 0 (strawman) I'm leery of using anything too experimental as it is more likely to cause breaking changes as the features hit a standards track. IIRC most of this app - if not everything used so far - is available in stage: 1, and I'd feel more comfortable sticking with >= 1 for now to mitigate possible growing pains as the specs settle.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

We should downgrade the stage plugin to account for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 especially for open source stuff.


if (this.props.className && this.props.className.indexOf('attached') !== -1) {
content = ::this.renderAttachedBar();
}

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

describe('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 a div with the class of progress', () => {
// render(<Progress />).firstClass('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.

I was having some trouble understanding the React-utils. This test was failing because it found two progress classes on <Progress />. I'd be interested in pairing on it.

Copy link

Choose a reason for hiding this comment

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

Me too! I'm still getting adjusted to the test utils and am interested in a pair here.

Copy link
Member

Choose a reason for hiding this comment

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

Hero me up folks, I'll clear this up for both of you.

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