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

Add Progress Component #121

merged 15 commits into from
Dec 7, 2015

Conversation

eanplatter
Copy link
Contributor

No description provided.

@levithomason
Copy link
Member

@eanplatter where are we at on this guy, look like it needs a push from your local?

@eanplatter
Copy link
Contributor Author

@levithomason this is up to date, I am not sure why it says those commits are 9 days old. It has the updated meta and tests.

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

@eanplatter eanplatter self-assigned this Dec 7, 2015

// 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.

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.

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.

@ghost
Copy link

ghost commented Dec 7, 2015

Please 💣 that .DS_Store file and consider adding to .gitignore. If you're using ST2/3 there's a plug-in called Gitignore to make generating ignore files quick and easy.

@levithomason
Copy link
Member

after removing DS_Store 🉑

);
};

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

@ghost
Copy link

ghost commented Dec 7, 2015

🍃

eanplatter added a commit that referenced this pull request Dec 7, 2015
@eanplatter eanplatter merged commit 488612f into master Dec 7, 2015
@eanplatter eanplatter deleted the feature/progress branch December 7, 2015 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants