felixhagspiel / jsOnlyLightbox

Responsive Lightbox in plain JS. No need for jQuery.
MIT License
157 stars 49 forks source link

Bug and React Router hack #35

Closed kswope closed 7 years ago

kswope commented 7 years ago

First off, the option 'boxId' passed to load(), which I'm later recommending using, is broken, it must not be very popular

lightbox.js : line 409

// load box in custom element
if (CTX.opt.boxId) { 
  CTX.box = document.getElementById(CTX.opt);  
}  

Should be

// load box in custom element                                                                                                             
if (CTX.opt.boxId) {                                                                                                                      
  CTX.box = document.getElementById(CTX.opt.boxId);                                                                                           
}  

Moving on...

I was getting a strange interaction with this library and react-router.

Normally I wouldn't use a javascript library that manipulated the DOM out from under React but since the DOM meddling is limited and 'modal' (nothing else happens at the same time) I figured it was safe enough, a lot less messy than existing React lightboxes, but there was one gotcha.

You have to use the boxId option

in componentDidMount()

new Lightbox().load( { boxId: 'lightbox_anchor' } )                                                                                           

in your index page

<span id='lightbox_anchor' class='jslghtbx'></span>                                                                                             
<script src="bundle.js"></script> 

The reason, simply put, is that Lightbox's load(), without the boxId, relies on the page being reloaded (DOM being reset), because it hangs an new element on it, and Lightbox expects it to not be there whenver load() is called.

The problem occurs when you use react-router, navigate away and back, or to a new page, which calls load() again, but that element is still there, blowing up the conditional logic within Lightbox.

existing lightbox, CTX.box never set because of lingering element 'jslghtbx'

   // load box in custom element                                                                                                                     
   if (CTX.opt.boxId) {                                                                                                                              
     console.log('CTX.opt.boxId', CTX.opt.boxId, document.getElementById(CTX.opt.boxId))                                                             
       console.log(document.getElementById('lightbox_anchor'))                                                                                       
       CTX.box = document.getElementById(CTX.opt.boxId);                                                                                             
   }                                                                                                                                                 
   // create box element if no ID is given                                                                                                           
   else if (!CTX.box && !document.getElementById('jslghtbx')) {                                                                                      
       var newEl = document.createElement('div');                                                                                                    
       newEl.setAttribute('id', 'jslghtbx');                                                                                                         
       newEl.setAttribute('class', 'jslghtbx');                                                                                                      
       CTX.box = newEl;                                                                                                                              
       body.appendChild(CTX.box);                                                                                                                    
   } 

possible fix, add another conditional

   // load box in custom element                                                                                                                     
   if (CTX.opt.boxId) {                                                                                                                              
     console.log('CTX.opt.boxId', CTX.opt.boxId, document.getElementById(CTX.opt.boxId))                                                             
       console.log(document.getElementById('lightbox_anchor'))                                                                                       
       CTX.box = document.getElementById(CTX.opt.boxId);                                                                                             
   }                                                                                                                                                 
   // element already exists in DOM ( SPA's, etc )                                                                                                                 
   else if (!CTX.box && document.getElementById('jslghtbx')) {                                                                                       
     CTX.box = document.getElementById('jslghtbx')                                                                                                   
   }                                                                                                                                                 
   else if (!CTX.box && !document.getElementById('jslghtbx')) {                                                                                      
       var newEl = document.createElement('div');                                                                                                    
       newEl.setAttribute('id', 'jslghtbx');                                                                                                         
       newEl.setAttribute('class', 'jslghtbx');                                                                                                      
       CTX.box = newEl;                                                                                                                              
       body.appendChild(CTX.box);                                                                                                                    
   }

Sorry no patches or PRs, I don't know this code well enough to trust myself with changing it.

felixhagspiel commented 7 years ago

@kswope thanks for the feedback & sorry for the late reply, busy times. I guess you are using an old version, as this is already fixed in 0.5.4. How did you install the lightbox? I forgot to update npm, shame on me (version there was 0.5.3). I just released 0.5.5 with a couple of other fixes, please try to update it. Regarding your react-router, I added a check which is called in setOpt to only create a div when no boxId param is given and if there is no element which already has the id jslghtbx. Please let me know if that helped, otherwise reopen the ticket.