Moved generation of notify div element from php code to hypha.html #172

Merged

giplt merged 4 commits from notifyStyling into master

2019-01-05 14:54:03 +00:00

giplt commented

2019-01-05 11:03:21 +00:00

(Migrated from github.com)

The div element with id 'hyphaNotify' was generated dynamically in the function
addNotifier in events.php and added to the end of the DOM document. Now it is
included in the default hypha.html document to facilitate styling.

The div element with id 'hyphaNotify' was generated dynamically in the function addNotifier in events.php and added to the end of the DOM document. Now it is included in the default hypha.html document to facilitate styling.

matthijskooijman (Migrated from github.com) reviewed

2019-01-05 13:01:46 +00:00

matthijskooijman (Migrated from github.com) left a comment

One way to fix this would be to keep the old code, but only insert the div when it is not already present. If we do this inside hypha_getHtml() (IIRC), this would also automatically happen when you edit the markup through the webinterface, so if you edit it once, it will automatically get inserted. Another approach would be to have some kind of explicit "migration" run that modifies all themes.

Or we could ignore this for now (just update it manually) and create something proper for migrations later.

I wonder what we should do with existing installations, that do not have this entry in their hypha.html. With the PR as it is now, these require manually updating hypha.html, or they will lose notifications entirely. One way to fix this would be to keep the old code, but only insert the div when it is not already present. If we do this inside hypha_getHtml() (IIRC), this would also automatically happen when you edit the markup through the webinterface, so if you edit it once, it will automatically get inserted. Another approach would be to have some kind of explicit "migration" run that modifies all themes. Or we could ignore this for now (just update it manually) and create something proper for migrations later.

system/core/events.php

This visibility setting should actually happen further down below, since now notifications from $_GET are not included in this check. I think this is not a new problem, though, but something that's easy to fix now.

matthijskooijman commented

2019-01-05 14:28:17 +00:00

(Migrated from github.com)

I added a very rough compatibility check, to at least not break existing installations.

I also fixed the GET notification ordering, which were indeed hidden. While I was there, I fixed a few more details.

I added a very rough compatibility check, to at least not break existing installations. I also fixed the GET notification ordering, which were indeed hidden. While I was there, I fixed a few more details.

laurensmartina (Migrated from github.com) approved these changes

2019-01-05 14:53:45 +00:00

laurensmartina (Migrated from github.com) left a comment

Looks good!

system/core/events.php