search

cubiq / add-to-homescreen

Add to home screen call-out for mobile devices
http://cubiq.org/add-to-home-screen
2.29k stars 747 forks source link

Move this.container assignment to DOM append logic #235

Closed jhrr closed 7 years ago

jhrr commented 7 years ago

I was having problems with getting the popup to show up at all and hacking around in the console, (by deleting local storage, setting shown to be false, reinstantiating the object etc.) it was clear that this.container in the ath.Class function for some reason was not setting this.container correctly, and so, when it came to appending the viewport node to the container I was getting a null error.

Looking around it seemed to make more sense to assign this.container in the same part of the code as the other DOM appending actions and doing this did in fact fix my problem; the popup now shows up for me as expected and behaves correctly.

I'm not sure quite why this was happening in the first place, but it seems to me that if it doesn't have any other impact elsewhere (I can't see that it does) it makes sense to just define the container along with the element and viewport nodes, like so...

jhrr commented 7 years ago

Right, so document.body is indeed null when I call addToHomeScreen in the head ("as soon as possible" as the docs suggest) and this is causing the null error when it comes time to append the popup to the container node. So, the body has yet to be defined when we make the early instantiation of addToHomeScreen. It does seem better to move that assignment to the show method with the rest of the DOM manipulation and it will then be behind the !_DOMReady guard on line 445.