search

juliancwirko / meteor-s-alert

Simple and fancy notifications for Meteor
https://atmospherejs.com/juliancwirko/s-alert
190 stars 23 forks source link

Multiple {{> sAlert}} inclusions #62

Open arist0tl3 opened 7 years ago

arist0tl3 commented 7 years ago

Hey @juliancwirko,

first off, thanks for this package! Super easy to implement and extend.

After pulling my hair out for an hour trying to figure out why I couldn't get an alert to appear above a dialog, I realized that per these specs, I'd never z-index an alert on top of the dialog.

So, my temp fix is to include {{> sAlert}} at the body level, and also inside of <dialog> components, if I need alerts to fire there. So far, things look good, but that's based on the jQuery method of cleanup that is currently in place in this package. A swap to document.getElementById would obviously break that functionality, as I have multiple alert boxes.

Just wondering if the jQuery selector was a conscious decision on your part, and if you had any thoughts on best practices here. I toyed with the idea of wrapping the body inclusion of {{> sAlert}} in some logic based on whether or not a dialog was visible, but then race conditions, waiting on dialog rendering, closing, etc. and having multiple template inclusions doesn't seem to be an issue currently.

Thanks again!

Sean

juliancwirko commented 7 years ago

sAlert template should be always placed as close to the root as possible. This package is quite old, this is probably why there is jQuery. It was an integral part of Meteor in the past.

I'll try to improve it, but it's hard to say when :/

If you'll use React try : https://www.npmjs.com/package/react-s-alert