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

Modal should inject .modal-content instead of .modal-body #9318

Closed
mediafreakch opened this issue Aug 10, 2013 · 10 comments

Comments

Projects
None yet
7 participants
@mediafreakch
Copy link
Contributor

commented Aug 10, 2013

I think that the modal plugin would benefit from the following enhancement:

when the remote option is specified (either using JS or data-remote attribute), the content should be loaded into .modal-content instead of .modal-body .

This would allow folks to customize the modal-header (including title) and footer as well and allow more flexibility and reuse of code...

What do you think?

@egobude

This comment has been minimized.

Copy link

commented Aug 10, 2013

+1

@cvrebert

This comment has been minimized.

Copy link
Member

commented Aug 10, 2013

Technically a duplicate of #5161, but to the extent that the remote option makes sense, I don't agree with the current behavior either, so good luck with the appeal.

@mediafreakch

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2013

Sry about that. Reading through the previous issues I see that this one will probably be closed as well. But can you at least give an explanation why? And tell us the workaround you have in mind? Maybe it's not needed, but I don't see any other simple way...

@fat fat closed this in 89b3cb0 Aug 11, 2013

@gabel

This comment has been minimized.

Copy link

commented Aug 13, 2013

This is broken now....

The remote url got loaded into the modal div directly, not in the modal-content

http://jsfiddle.net/H5XhK/1/

BTW:
In my op this change does not make sense (if it would work as expected). If I write one "myModal" HTML snippet that I can reuse with several remote href sources/links with data-toggle="modal", then I need to change all sources now, instead of adding another title via JS by the show(n) event and load the remote sources into the modal-body. This brings code duplication of the modal-header for me instead of reuse.

@jslaybaugh

This comment has been minimized.

Copy link

commented Aug 14, 2013

+1 on @gabel's comment. For a while, I've wanted to be able to customize the header and footer on load, but I've discovered (as he says) it really duplicates a lot of HTML and makes my partial bound too strongly to the modal structure instead of being something that can be injected anywhere (.modal-body being just one of those places).

Plus, beyond those semantics, he's also right about it being broken as-is. It should insert into .modal-content if it is going to remain as is.

In general, I know you guys aren't fond of cluttering up the data- attributes unnecessarily but for the sake of backwards compatibility and the other reasons listed above, why not keep it inserting into .modal-body and then maybe add another data- attribute like data-destination (or something) to actually specify where they want it inserted. It would be backwards compatible and just as versatile for those who want it.

@cvrebert

This comment has been minimized.

Copy link
Member

commented Aug 14, 2013

This is broken now....

It breaks old code. Such is the very nature of backwards-incompatible releases, like BS v3.

[...] but for the sake of backwards compatibility [...]

v3 breaks an absolute crap-ton of things already.
So, IMHO: 1 more incompatibility isn't gonna kill anybody, and we haven't "wasted" time writing compatibility shims for anything else, so I don't see much reason to start doing so here.
But hey, I'm not the decider, so feel free to open an issue or pull request regarding data-destination; maybe it'll get traction from mdo or fat.

If I write one "myModal" HTML snippet that I can reuse with several remote href sources/links with data-toggle="modal", then I need to change all sources now, instead of adding another title via JS by the show(n) event and load the remote sources into the modal-body.

If you look at the issues related to this one, you'll see that lots of people want the new behavior and would prefer not to have to write any custom JS. Since one of Bootstrap's main fanbases is backend devs who want to quickly slap a spiffy frontend on something, this desire seems reasonable in context. And, in general, the modal title being based on server-side data is also a fairly common case.
IMO, if one wants to do fancier things, one should use custom JS (e.g. client-side templating, perhaps still incorporating a fetch from the server if you're into that for some reason) instead of data-remote; data-remote already seems kinda crippled, so trying to do anything nontrivial with it is probably going to be painful.

Plus, beyond those semantics, he's also right about it being broken as-is. It should insert into .modal-content if it is going to remain as is.

This complaint seems entirely legit; I'd suggest opening a new issue about it.

@Eagllus

This comment has been minimized.

Copy link

commented Sep 2, 2013

I still would like to know if there is a good clean work around for this problem?
I do about 2 requests for systems logs from around 100+ different servers.
So the options to change the output of a different application that returns the log is not an option.

For now I will override the modal function so that it will put the content into the modal-body but I would like to have a clean solution for this. Any tips or idea's about this would be greatly appreciated.

@cvrebert

This comment has been minimized.

Copy link
Member

commented Sep 2, 2013

@dreagle Overriding is fairly clean. (But are you HTML-escaping the presumably-plain-text logs?) Or just don't use data-remote at all; do the fetch + injection/templating yourself (see above comment).

@Eagllus

This comment has been minimized.

Copy link

commented Sep 3, 2013

I found the solution easier by taking the modal file from version 2.3.2 and use this one for only the modal calls.
But thanks for the advise!

@AaronLS

This comment has been minimized.

Copy link

commented Oct 18, 2013

Instead of hardcoding to replace body, or content, etc., have it replace the inner of whatever is selected with data-target?

fat added a commit that referenced this issue Dec 19, 2013

fat added a commit that referenced this issue Dec 24, 2013

fat added a commit that referenced this issue Dec 24, 2013

Merge pull request #11933 from twbs/fat-10105
change where modal loads content -– fixes #10105, #9318, #9459

@mdo mdo referenced this issue Dec 25, 2013

Closed

v3.1.0 ship list #11734

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.