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] Allow custom Modal dialog components #1060

Merged
merged 1 commit into from
Jul 30, 2015
Merged

Conversation

jquense
Copy link
Member

@jquense jquense commented Jul 24, 2015

pulled out from #1040, needs some additional bike shedding

cc @mfunkie

@taion
Copy link
Member

taion commented Jul 26, 2015

Reprise - the question to me is whether we like the current API better, or if it would be better to do e.g.

<Modal customDialog={true} ... >
  <MyModalDialog />
</Modal>

@AlexKVal
Copy link
Member

I vote for @taion proposal:

<Modal customDialog={true} ... >
  <MyModalDialog />
</Modal>

I'm proposing the same API for another issue: #1071

There are two versions of API you can compare #1071 and #1072

@AlexKVal
Copy link
Member

Matt also is a proponent of such API 😄 #748 (comment)

@taion
Copy link
Member

taion commented Jul 29, 2015

I retract what I said earlier.

For handling the "generic modal" use case, it makes more sense to split out a new package from this with the core utilities around e.g. Modal, Overlay, Transition, Position, and friends.

It makes more sense for the Modal component in React-Bootstrap itself to map well to the Bootstrap modal, which this accomplishes quite well, while still giving some flexibility for customization.

I'm okay with merging this as-is. Will open a new issue regarding pulling out core components.

@taion
Copy link
Member

taion commented Jul 30, 2015

@jquense Can you rebase this? Let's merge this and defer the generic API to #1084, unless there are further objections.

@taion taion force-pushed the custom-dialog-api branch from 36e87d7 to 0f46a97 Compare July 30, 2015 17:54
@taion
Copy link
Member

taion commented Jul 30, 2015

I rebased - the conflict was spurious (some overlapping lines with another added test).

@jquense
Copy link
Member Author

jquense commented Jul 30, 2015

I am fine with this as is if you are.

taion added a commit that referenced this pull request Jul 30, 2015
[added] Allow custom Modal dialog components
@taion taion merged commit b64a7a3 into master Jul 30, 2015
@taion taion deleted the custom-dialog-api branch July 30, 2015 17:58
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.

3 participants