search

maciej-gurban / responsive-bootstrap-toolkit

Responsive Bootstrap Toolkit allows for easy breakpoint detection in JavaScript
MIT License
363 stars 89 forks source link

Bugfix: DOM elements werent appended to body in Firefox + Bootstrap3's f... #9

Closed felixmaier1989 closed 9 years ago

felixmaier1989 commented 9 years ago

According to Bootstrap3's doc (http://getbootstrap.com/css/#responsive-utilities):

The classes .visible-xs, .visible-sm, .visible-md, and .visible-lg also exist, but are deprecated as of v3.2.0. They are approximately equivalent to .visible-*-block, except with additional special cases for toggling table-related elements.

We should insert classes visible-*-block to ensure forward compatibility.

maciej-gurban commented 9 years ago

I tested version 3.3.4 of Bootstrap stylesheets on Firefox 36.0.3 and everything works as expected. If you've found an edge case where it doesn't work, could you try to reproduce the problem by creating a jsfiddle and providing more information on the problem?

Unfortunately, I can't accept your pull request, because I failed to reproduce the problem it's meant to fix.

Thanks for pointing out the visibility classes changes. I'll introduce them in the new version, along with a few more features.

Edit: Also, if I'm reading your code correctly, you're appending a div to body during every execution of is() method.

felixmaier1989 commented 9 years ago

I'll try to make it reproducible.

felixmaier1989 commented 9 years ago

Hi Maciej,

Unfortunately I could not reproduce the bug on jsfiddle. But you can download an HTML file under https://www.wetransfer.com/downloads/63a46c3afe96fe62074b44a2796eab1720150413090316/23c0c69f67ce99db32847dec495eb3c420150413090316/2ea3ce where the bug should occur. I tested it under two different versions of firefox and chrome.

Regards

maciej-gurban commented 9 years ago

Hi Felix,

Thanks for the example. The reason why the standard .is() method does not work for you and works in the example, is because you've included the library in <head> section, while the example uses <body>.

In your case, there's no <body> to append to yet, at that point, hence the broken functionality. Try moving your script and library inclusion to <body> and it will start working.

Now for cases, where for some reason you'd have to keep your script and library inclusion in <head>, here's a temporary solution:

The last line of Responsive Bootstrap Toolkit is })(jQuery);. This means everything within ResponsiveBootstrapToolkit expression will be evaluated immediately. You can remove (jQuery) to postpone the execution until the point you're sure the document has been fully initialized.

We'll run ResponsiveBootstrapToolkit after document has fully loaded as follows:

$(document).ready(function() {
    (function($, viewport){

        if (viewport.is('xs')) {
            alert('XS');
        }

      })(jQuery, ResponsiveBootstrapToolkit(jQuery));
 });

Thanks for pointing out this behaviour. I'll take a look at a cleaner solution some time soon.

Best regards, Maciej

felixmaier1989 commented 9 years ago

Thanks for your solution.

As moving our JS code after the <body> tag is still in our pipeline, I wont dig any further and use my version as it is, more or less...

...I was indeed appending too many div's https://github.com/felixmaier1989/responsive-bootstrap-toolkit/commit/c9e160ede57345d4dda8201baa99773db50ff3ab