Alternative Backbone implementation for Alert UI elements. #204
Conversation
This reverts commit e4d2047.
Because JavaScript is a fucking mess, and this also DRYs the front-end testing, Karma will always report failure when running JS tests since the new global JavaScript object to hold the DOM hooks (`buoy_dom_hooks`) isn't being loaded in the test runner's container. Since this object is now dynamically constructed by using WordPress's `wp_localize_script()`, I don't actually know how to include this in the test runner. So, it'd be nice if we figured that out at some point. Meanwhile, we can either: * Repeat ourselves in the test runner and just *hope* things work. * DRY the tests and manually duplicate a JS object in the test runner. I hate both of these options so basically my approach right now is: fuck JavaScript, and fuck these tests, since we're not actually failing builds with the JS tests in Travis (yet) anyway. shrug.gif and stuff.
* Marks BackboneJS as a dependency of Buoy, so WordPress will load it. * Creates a `Backbone.View` for the "Activate Alert" screen. * Migrates JavaScript code from the `buoy-alert.js` module to Backbone. * Renames ID for the Immediate Alert button to `immediate-alert-button` * Updates all stylesheet references to point to this new ID. * Removes this ID from `buoy_dom_hooks`, not needed with Backbone! * Re-orders "main" script file code to group TODO tasks more logically. This is just a beginning to show how we can *simply* (i.e., gradually and without adding any complexity) refactor the existing client-side code while also avoiding creating dependencies on JavaScript. TL;DR: We can use Backbone to structure our code without requiring the use of JavaScript. Buoy currently can (and should continue to remain) usable *WITHOUT JAVASCRIPT ENABLED*.
This commit continues in the pattern from the previous commit, removing the `buoy_dom_hooks` where they are no longer needed and converting each individual user interface element to a `Backbone.View` object. Notably: * Alert button names have been standardized across the codebase. * The `attachHandlers` method was replaced with Backbone constructors. * Bug in `class-buoy-alert.php` when scheduling Timed Alerts was fixed. * JSDoc syntax is now used thoroughly across the `alert.js` file. Yay! This isn't perfect yet. There are a few parts where the lack of a real `Backbone.Model` for our alerts are requiring some hacky things. But the purpose of this commit was to begin the migration to a Backbone-based structure as gradually as is sensible to do so. Models will come later.
This commit includes BackboneJS as part of our `package.json`'s `devDependencies` and configures Karma to load it as necessary. This means we are mostly ready for testing Backbone components when and if we figure out what, exactly, we'd like to test. Previous to this commit, there was a rather contrived test for a single method. However, that was only ever being used in one place, so I've refactored it and removed it from the test suite as it's a single jQuery call, anyway.
| * In this file, we create a global `BUOY` object using the (classic) | ||
| * {@link http://www.adequatelygood.com/JavaScript-Module-Pattern-In-Depth.html|JavaScript Module Pattern} | ||
| * and return a simple interface to interact with {@link http://BackboneJS.org|Backbone} | ||
| * library components (`Models`, `Views`, and `Collections`), alon with |
There was a problem hiding this comment.
Yeah. ¯\_(ツ)_/¯ I'm gonna leave this here so that a future contributor who's less familiar with GitHub/git can see it, be annoyed by it, and try to fix it. Fixing typos inside of comments is a great "first patch" sort of exercise.
| * Initializer. | ||
| */ | ||
| var init = function () { | ||
| // Note: This works around GitHub issue #47. | ||
| // Could be removed after WebKit and/or Bootstrap fixes this in their libs. | ||
| if (jQuery('.dashboard_page_buoy_chat, .dashboard_page_buoy_activate_alert').length) { | ||
| if (jQuery(buoy_dom_hooks.page_chat + ' ' + buoy_dom_hooks.page_activate_alert).length) { |
There was a problem hiding this comment.
I don't feel like we're gaining much here in terms of readability with buoy_dom_hooks. I would prefer a named boolean helper function like:
function pageHasActiveAlert() {
return jQuery(buoy_dom_hooks.page_chat + ' ' + buoy_dom_hooks.page_activate_alert).length > 0;
};if (pageHasActiveAlert) {
// Do the things
}Such helpers could even be defined on buoy_dom_hooks potentially. I'm not sure if there's a more Backboney way to handle this stuff.
There was a problem hiding this comment.
This has been my argument against buoy_dom_hooks in the first place. There is a more Backbone-y way to do this; note that I've completely removed the subsequent if conditionals from this file.
That said, this is literally a one-liner and it's a workaround for a bug in Bootstrap. I don't think a named helper, in this case is needed, specifically because this is code that we'll be removing anyway once Bootstrap fixes their bug.
| wp_localize_script(self::$prefix.'-script', self::$prefix.'_vars', self::localizeScript()); | ||
| // Localize it. | ||
| wp_localize_script(self::$prefix.'-script', self::$prefix.'_vars', self::localizeUiText()); | ||
| wp_localize_script(self::$prefix.'-script', self::$prefix.'_dom_hooks', self::$dom_hooks); |
There was a problem hiding this comment.
Would you mind explaining this for my wordpress education? I was able to understand from the WP codex why the ui text would go through wp_localize_text, but I don't totally understand why the dom hooks thing is in here.
There was a problem hiding this comment.
Would you mind explaining this for my wordpress education?
Sure. I'm going to take the opportunity to write up a more detailed explanation than you might have expected or maybe even need, consider it project documentation for future contributors if that's so.
I was able to understand from the WP codex why the ui text would go through
wp_localize_text, but I don't totally understand why the dom hooks thing is in here.
WordPress has only one mechanism in its API for getting PHP-generated values into the running JavaScript context of a page, and that mechanism is called wp_localize_script(). It was originally intended for client-side string interpolation in the context of localization, hence the name. The user's chosen locale, stored in the WordPress database, is queried by WordPress core, which then localizes all internationalized strings with its own localization API (built on GNU Gettext under the hood). But if these strings are needed by JavaScript, rather than simply being output to the page's HTML source, then they need to be injected into the JavaScript context somehow.
So the pipeline is:
database value -> PHP value -> string interpolation (localization, in this case) -> pushed as global JavaScript object
WordPress also has its own asset queue API, which is responsible for managing dependencies between client-side assets such as JavaScripts and style sheets. To be used, each asset must first be registered with WordPress (by something like a call to wp_register_script()). "Registration," to WordPress, simply means telling WordPress about the URL of the script, giving it a name (called a "script handle," or more generally an "asset handle," in WordPress jargon) and optionally supplying additional metadata such as the version of the asset, any dependencies it has on other assets, and so on. Dependencies are listed by referring to the handle of the previously-registered asset. Here is a list of all client-side assets and their handles shipped by WordPress core.
This mechanism is how we're actually including Backbone in the first place.
Anyway, once an asset like a script is registered, it may get "enqueued," which means WordPress will take responsibility for actually adding it inside of a <script> element in the HTML page at some point. This usually happens by a call to wp_enqueue_script(). Sometimes a script (or other asset) is registered and enqueued in one go, rather than in separate steps.
For "localization," however—and, importantly, this includes any operation wherein the client-side JavaScript values are determined dynamically from the server-side PHP code—we first have to call wp_localize_script() ourselves. This is kind of like registering a new script, but actually behaves more like telling WordPress about even more metadata that it should associate with a given already-registered script. In this case, though, the additional metadata isn't version strings and so on, it's the dynamically generated values.
By calling wp_localize_script() and associating it with a given handle, WordPress will not only output the original script's source to the browser, it will also output a <script> element whose contents is a JavaScript representation of the PHP array that the wp_localize_script() was passed. Once that's done, the JavaScript source itself can access these dynamically-generated values in the JavaScript context by looking for a global whose name matches the second argument to wp_localize_script().
TL;DR: Even though we're not technically "localizing" the buoy_dom_hooks strings, they are stored in PHP. Since they're stored in PHP, the cleanest way to move them into a JavaScript context is to use WordPress's localization API. The fact that the name of that function doesn't really match the use of that function in this case is an unfortunate consequence of WordPress's imperfect naming scheme and historical evolution.
| BUOY.Views.ImmediateAlertButton = Backbone.View.extend({ | ||
| 'events': { | ||
| 'click': function () { | ||
| this.$el.prop('disabled', true); // prevent double-clicks |
There was a problem hiding this comment.
I didn't notice this before but shit thank you for preventing double submissions. So many annoying bugs come from that.
Since we do this a lot, I spent a while considering whether a simple helper function could be extracted to do this, but ultimately decided I don't think it's worth the added complexity.
There was a problem hiding this comment.
I spent a while considering whether a simple helper function could be extracted to do this, but ultimately decided I don't think it's worth the added complexity.
Yeah. Also, a helper doesn't really make sense, at least not yet, because there's no place to put it. We could make a BUOY.helpers object but then we have technical overhead in terms of connecting that helper to the appropriate Backbone.View instance or passing the View to the helper. That's a lot of work for a one line thing.
It would also add cognitive overhead: we need to name the helper, trace the code to the helper when reading it, and so on. It's definitely too much for something as simple as a .prop() call handled by the View itself.
Finally, it's arguable that the View itself should be responsible for knowing that "I shouldn't be double-clicked." This is the correct place, conceptually, for such a determination.
| */ | ||
| BUOY.Views.ContextualAlertModal = Backbone.View.extend({ | ||
| 'events': { | ||
| 'click button.btn-success': function (e) { |
There was a problem hiding this comment.
I'm slightly bummed about the proliferation of this classname across several Backbone views. My understanding is that the event target is a child element of this.$el? I don't actually know what the page markup looks like. Each of these modals has their own button.btn-success I assume. I think part of what's grossing me out is the concatenation of the event string and the selector. In theory we could use a variable from buoy_dom_hooks to construct the key of the event hash but I think that's overkill. I should just suck it up and accept this convention as the lesser of dom spaghetti evils.
There was a problem hiding this comment.
Yeah, this is conventional Backbone-style event delegation. So, you'll just have to suck up, I'm afraid. One thing we can do (and that we are doing) to make this a little less gross is that we're using conventional markup and behaviors provided by Bootstrap (as linked in the modal View's JSDoc docblock) to make sure that this Backbone component always knows about its own markup. The point here being that the markup itself will not change, because it's defined by Bootstrap, and so the Backbone View can know about the underlying markup of the interface component it's responsible for progressively enhancing, without relying on any other part of the codebase.
After trying it out (in 032bc1f, c86f8cd, and 533d82a), I have a lot more distaste for the buoy_dom_hooks pattern thingy because it does not actually DRY the code and, moreover, it introduces a PHP dependency into the JavaScript. That is far, far worse, IMHO, than simply maintaining a conventional mapping between a Backbone View component and the HTML DOM for which it is responsible, which feels much more in keeping with Single Responsibility anyway. It isn't perfect but dramatically reduces our edit-per-change ratio (how many places in the code we need to edit to maintain equivalent functionality for an arbitrary single change), as proven by this PR, I think. That's the measurement of paramount importance to this refactor in the first place.
This PR includes #190 and takes some of the insight provided by the Backbone implementation in #199 to provide an alternative, smaller, and more conventional Backbone implementation for the Activate Alert Screen and other UI elements (38e8217 and 1292daa).
Also included in this PR are commits related to the following:
devDependenciesto prepare for future testing (7fabde0).