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

introduced new component Carousel.Caption #1734

Merged
merged 2 commits into from
Mar 30, 2016

Conversation

abloomston
Copy link

Carousel.Caption replaces the following bootstrap code:

<div className="carousel-caption">

cf. Media.Body from #1707 for a similar change.

Carousel.Caption replaces the following bootstrap code:

```<div className="carousel-caption">```

cf. Media.Body from react-bootstrap#1707 for a similar change.
@abloomston
Copy link
Author

If you are including Carousel as follows before this change:

const Carousel = require('../../src/Carousel');

You will receive the following error when e.g. using webpack:

Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.

To fix this, change the require to this:

const Carousel = require('../../src/Carousel').default;

export default Carousel;

export { Caption };
Copy link
Member

Choose a reason for hiding this comment

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

this amounts to a breaking change, so i'd either need to go in the 0.29 branch or (preferablly) just remove the named export since the Static property on Carousel is fine

Copy link
Author

Choose a reason for hiding this comment

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

The downside is a breaking change, the upside is consistency with e.g. Media.

That being said, I defer to you. Let me know which you'd like me to do, PR to 0.29 branch or modify the code.

Copy link
Member

Choose a reason for hiding this comment

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

That's a mistake on <Media>. We'll fix that.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@taion taion mentioned this pull request Mar 30, 2016
@taion
Copy link
Member

taion commented Mar 30, 2016

LGTM

1 similar comment
@jquense
Copy link
Member

jquense commented Mar 30, 2016

LGTM

@jquense jquense merged commit 02c44ce into react-bootstrap:master Mar 30, 2016
@abloomston abloomston deleted the feature-Carousel.Caption branch March 30, 2016 18:03
@taion
Copy link
Member

taion commented Mar 30, 2016

I wonder if we should have <Carousel.Item>.

@taion
Copy link
Member

taion commented Mar 30, 2016

#1740

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

3 participants