Add in a little UI sugar to warn users when someone else is editing the same page. #8

Merged
merged 5 commits into from Jun 13, 2012

Conversation

Projects
None yet
4 participants

No description provided.

+ <li class="minibutton"><a href="/history/{{escaped_name}}" class="action-page-history">Page History</a></li>
+ </ul>
+ </div>
+ <div id="warnings"></div>
<div id="wiki-content">{{>editor}}</div>
</div>
<script type="text/javascript">
@clowder

clowder May 29, 2012

Can we extract the JS out into its own file.

Would be nice to have it wrapped in a (function(jQuery) {})(jQuery), so that we aren’t adding everything to the global namespace.

@dazoakley

dazoakley May 30, 2012

Ok, debate time...

I will happily do this, but I want your arguments as to why, and how this will improve the quality of the code/application.

Here's my reasons for doing it as I have:

  • It's code that is only ever going to be used on this page - why hide it away in another file?
  • It's not that much code so it's not going to affect performance.
  • It uses variables only available in the mustache templates - to extract out this code we'd need to create a global config object or something in order to house these variables, (which are only going to be used here), and then have the extracted code read them from there. To me this smells of making the code far less straight-forward to read and a bit of over-engineering for what is a simple bit of functionality...

As for the wrapping it in an anonymous function, we can do that but again, it smells of over-engineering for the sake of it - we're not building a library here, just adding some functionality to a page, adding two functions to the global namespace will not hurt.

@clowder

clowder May 30, 2012

There are a couple of angles here.

Firstly, we should be treating our JS as a first class code citizen. Meaning it should have its own unit test running in isolation independent of the pages that the code is used in.

Flowing on from that, its a slippery slope. “Lets just inline it this once” opens the door to having javascript scatter shotted all over the application. If we move towards centralising our javascript we then have the option to do a little funky JS compression and concatenation etc. (something that this page is in need of) to make sure that we are getting the lightest pages possible.

Secondly for the closure, the idea is to get all the functions out of the global namespace and only expose the parts of your API to the page that it needs. Also, encapsulating your API protects it from external factors. For example I include a new javascript file ahead of yours thats also a bad JS citizen and refine ping_interval to be a function... BOOM (I know its a trivial example, but as the application grows adding code to the global namespace will increase the risk of silly bugs like this).

@bravoecho

bravoecho May 30, 2012

Hi, of course I can't retreat from a javascript debate :-)

In general the use of setInterval is discouraged because if the computations inside the given function take more than the interval, the executions of the same function will overlap. The common advice in this case is to use a setTimeout that at the end of the function invokes itself. An interesting discussion here: http://stackoverflow.com/questions/729921/settimeout-or-setinterval

As to wrapping ALL the things in a closure, it's considered a no-brainer, if I have the right understanding of the feelings of the community about it.

The main reasons I can tell off the top of my head to do are mostly related to the "don't make me think pattern"

  • if the code is not in a file, I can't easily discover it when I look inside the javascript directory
  • I can't easily track the dependencies
  • I can't easily validate it (if it's in a file I can fire a JSLint run in most editors)

For example, the first thing I wonder is where the faye_client_with_auth comes from: of course I can grep it, but it's probably a good thing to make that dependency explicit.

From the strictly technical stand-point I agree with Chris. I would also remind that I couldn't enable strict mode, because if I do it in the global scope, then all the code in the page will be interpreted as strict, even if it isn't designed to be "strict-safe"

Generally speaking me thinks that is a good thing to expose in the global space only the reciprocal dependencies (as long AMD is not used, which is fine), not the business-specific code.
And even in that case, putting everything inside a namespace (properties of a single global object) so I can easily explore what really available to the user of the global modules.

Let me know if this makes sense... :-)

@bravoecho

bravoecho May 30, 2012

Sorry, just being bitten by one of the side effects of js in the page, so it's a good opportunity to share the idea.

One of the main issues is that if you load your dependencies (jQuery and so on) from an external file, there is no guarantee that the functions defined in the external files will be available at the time when your inline script will be executed. I'm having problems right now in webistrano, because if I try to organize the files in a different way (for example putting jQuery in the default jammit package) jQuery is not available to many of these inline scripts, so they will all fail.

@mudge

mudge May 30, 2012

Just in case there weren't enough cooks yet:

  • The closure should definitely be added as faye_client, others_editing and ping_interval are effectively global variables at the moment;

  • The larger question of whether this should be extracted to a separate file or not is more interesting. Of course, compression might not make a huge saving and maybe you have are fluent with ack for finding JavaScript but the assumption I would challenge is why is this so specific to this one page?

    Surely, this functionality is actually quite useful: being notified of other users on the same page simultaneously is something that could be used in myriad ways.

    Perhaps the fact that we are so inherently coupled to the variables passed from Mustache is actually a smell. What if, instead, you included some some sort of reusable plugin and then did something like:

(function ($) {
  $(document).ready(function () {
    $.GollumEditor();
    NotifyPresence({
      client_id: '{{faye_client_id}}',
      token: '{{faye_auth_token}}',
      user: '{{current_user_name}}',
      email: '{{current_user_email}}',
      page: '{{escaped_name}}'
  });
}(jQuery));

(Warning: fatuous example above -- particularly as it doesn't let you customise the message -- but you get the gist.)

Here's v2...

lib/gollum/frontend/public/gollum/javascript/notify_presence.js
+ }
+ }
+
+ setTimeout(function() { clear_old_warning_messages() }, ActiveOptions.interval);
@dazoakley

dazoakley May 31, 2012

setTimeout( clear_old_warning_messages, ActiveOptions.interval);
lib/gollum/frontend/public/gollum/javascript/notify_presence.js
+ }
+ );
+
+ setTimeout(function() { notify_others_of_editing() }, ActiveOptions.interval);
@dazoakley

dazoakley May 31, 2012

setTimeout( notify_others_of_editing, ActiveOptions.interval);

Hold on merge until the Faye server is alive...

@clowder clowder merged this pull request into master Jun 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment