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

[added] Images Component with Sample and Docs #1293

Merged
merged 1 commit into from Sep 20, 2015

Conversation

deerawan
Copy link
Contributor

@deerawan deerawan commented Sep 6, 2015

Create an images component as refer to http://getbootstrap.com/css/#images including samples and docs.

thumbnail: React.PropTypes.bool
},

getDefaultProps() {
Copy link
Member

Choose a reason for hiding this comment

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

I thought our prop tables were generated off prop types, not default props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @taion for the review. Does it mean the getDefaultProps() method is not needed here?

@taion
Copy link
Member

taion commented Sep 7, 2015

See inline comments. Also, we'd want this squashed into a single commit with a comment like: [added] Image component

image testing done

add docs

add comment for props

* Merge previous three samples (ImageCircle, ImageRounded, ImageThumbnail) to one sample file (ImageShape)

* Add ImageResponsive and ImageShape in ReactPlayground.js

* Add Image source include in Samples.js

fix the comment

* Fix eslint issue on Image.js
* Fix eslint issue on ImageSpec.js

* remove src and alt
* refactor test code to test attribute not props

fix eslint
@AlexKVal
Copy link
Member

LGTM. I'm fine to place it to the end of Components page.

Need one more sign of @react-bootstrap/collaborators

Docs part looks like:

screen shot 2015-09-19 at 8 00 43 pm

screen shot 2015-09-19 at 8 00 49 pm

@taion
Copy link
Member

taion commented Sep 19, 2015

LGTM, but would we be okay with putting this above "Utilities" at least?

@AlexKVal
Copy link
Member

Ok. Then I merge it and I will move it upper and fix eslint warnings fixing because of #1335.
I'll move it right before Thumbnail component. Right ?

AlexKVal added a commit that referenced this pull request Sep 20, 2015
[added] Images Component with Sample and Docs
@AlexKVal AlexKVal merged commit 1463e98 into react-bootstrap:master Sep 20, 2015
@deerawan
Copy link
Contributor Author

@AlexKVal thanks for your fixing :)

@AlexKVal
Copy link
Member

Np 🍒

@AlexKVal
Copy link
Member

Thank you @deerawan for contributing to this project ✨ ❇️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants