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

change where modal loads content -– fixes #10105, #9318, #9459 #11933

Merged
merged 1 commit into from
Dec 24, 2013
Merged

Conversation

fat
Copy link
Member

@fat fat commented Dec 19, 2013

@mdo @cvrebert

this breaks backwards compat… but i think we should make this change…

conversations here: #10105, #9318, #9459

maybe it's fine to put in next release if we message it well? o_O

otherwise i have to do a p nasty hack

@mdo
Copy link
Member

mdo commented Dec 19, 2013

I can see shipping this with v3.1 since I see the current problem as a bug (or at least an oversight) for those using remote content. Messaging it clearly enough in the release blog post should suffice I would think.

Thoughts, @cvrebert?

@cvrebert
Copy link
Collaborator

Oh how I would love to deprecate the remote option entirely...
Maybe we should change the readme to say that we "roughly" follow SemVer?

@mdo
Copy link
Member

mdo commented Dec 19, 2013

Maybe we should change the readme to say that we "roughly" follow SemVer?

It already does 😆. From https://github.com/twbs/bootstrap/blob/master/README.md#versioning:

For transparency into our release cycle and in striving to maintain backward compatibility, Bootstrap is maintained under the Semantic Versioning guidelines. Sometimes we screw up, but we'll adhere to these rules whenever possible.

Emphasis mine.

@cvrebert
Copy link
Collaborator

I guess I had instead interpreted that as "We won't intentionally violate SemVer, but we might accidentally".

@fat
Copy link
Member Author

fat commented Dec 24, 2013

yeah… i never was crazy about the remote option… but people really wanted it…

@fat
Copy link
Member Author

fat commented Dec 24, 2013

im going to mergggge… don't forget to add to blog post ya? ❤️

fat added a commit that referenced this pull request Dec 24, 2013
change where modal loads content -– fixes #10105, #9318, #9459
@fat fat merged commit 237b85a into master Dec 24, 2013
@fat fat deleted the fat-10105 branch December 24, 2013 07:31
@mdo
Copy link
Member

mdo commented Dec 27, 2013

@fat I've got it <3.

@mdo mdo mentioned this pull request Dec 27, 2013
@mattzuba
Copy link

Is it just me, or was this change not highlighted in the docs for the modal plugin? The docs still say the modal will be loaded in the root modal element.

"If a remote URL is provided, content will be loaded one time via jQuery's load method and injected into the root of the modal element"

@juthilo
Copy link
Collaborator

juthilo commented Jan 31, 2014

@mattzuba Could you please open a new issue about that, so we can keep track of it? :)

@mdo mdo mentioned this pull request Feb 3, 2014
1 task
@BrunoWinck
Copy link

It caused no error , it just silently failed
I was lucky to read this before upgrading

So basically I removed

    <div class="modal-dialog">
        <div class="modal-content">
..
        </div>
    </div>

from the pages served behind the remote URI
and placed it inside the <div class="modal" ...>

What is wrong with using remote ?

The "one time" is also wrong, it depends on cache pragmas, not on boostrap.

So doc should be
"If a remote URL is provided, content will be loaded via jQuery's load method and injected into the/every modal-content descendant of the modal element if one/they exists."

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

Successfully merging this pull request may close these issues.

6 participants