Problem/Motivation
After several months of use, it has become evident that the Toolbar JavaScript code is unstable and buggy in several places. We have been porting the more JavaScript-complex modules to Backbone.js. For example contextual and edit. Toolbar will benefit as well from a structured JavaScript foundation that models state more robustly.
Proposed resolution
Refactor the code onto Backbone.js.
Remaining tasks
None.
User interface changes
None.
API changes
Originally, this issue was intended to introduce a JavaScript interaction layer with the Toolbar. It has evolved into a simple refactoring onto Backbone.js, something we've done for other modules in core. There should be no API changes in this issue.
Comment | File | Size | Author |
---|---|---|---|
#79 | drupal-refactor-the-toolbar-1860434-79.patch | 45.98 KB | markhalliwell |
#79 | interdiff.txt | 929 bytes | markhalliwell |
#66 | interdiff_50-to-66.txt | 3.48 KB | jessebeach |
#66 | toolbar-backbone-1860434-66.patch | 46.25 KB | jessebeach |
#66 | Drupal_Core__D8__-_Status__1_Local_Change_.png | 34.09 KB | jessebeach |
Comments
Comment #1
Wim LeersI wonder if it'd be appropriate to leverage Backbone here? What's currently in the issue summary obviously related to state, and reacting to state. If captured in a Backbone Model, with the toolbar just being a View, wouldn't that simplify things a lot?
Comment #2
jessebeach CreditAttribution: jessebeach commentedMy fear is that this would require Backbone to load on all admin pages. I think we can get the same behavior with slightly-more-clever JS than what's there now.
Comment #3
Wim LeersMany parts of the back-end are slow anyway. Backbone + Underscore is 10.3 KiB. If you use Edit (which you likely do if you're an administrator), you're going to need Backbone + Underscore anyway.
So: does it really outweigh that?
I agree with your POV, I'm just not sure if it's really such a big difference overall.
Comment #3.0
Wim Leersadded a bit about an a11y consideration
Comment #4
jessebeach CreditAttribution: jessebeach commentedThe JS in the Edit and Tour modules convinced me that we should be using Backbone in Toolbar. It makes the code much easier to read and less unstable.
This patch ports the toolbar.js code to Backbone.
I'll be further enhancing it with better event hooks for modules to attach behaviors to.
Comment #5
webchickTagging as something we're working on this sprint.
Comment #6
jessebeach CreditAttribution: jessebeach commentedThis patch completes the Backbone port.
I also start exploring how we might enable communication between the toolbar items so that the state of the toolbar is properly maintained across the disparate modules that have tabs in it.
Comment #7
mgiffordPlease let me know when it is in a good place to test it. Also, any reason not to mark it needs review to get the bot to look it over?
Comment #8
jessebeach CreditAttribution: jessebeach commentedmgifford, thank you for the attention. It really needs a code review more than a functional test right now. I need someone to check my approach and make sure I'm not off the rails so far.
Comment #9
Wim LeersChanges
Drupal.behaviors.toolbar.attach()
was very dense due to zero blank lines. It may well be that Drupal uses too much blank lines too often, but here, especially combined with indented chaining, the code becomes very hard to parse for humans.StateModel
'smql
tomqMatches
, so that it's a simple boolean instead of a complex object. This simplifies code elsewhere (e.g. no more need for manual triggering of the change event), and reduces the amount of data in the model. Since Toolbar's JS is using only a single media query (to determine tray orientation), I think this should be okay.Drupal.behaviors.toolbar.options
was labeled as being "default options", hence I renamed it toDrupal.behaviors.toolbar.defaults
.model
andview
were "toolbar.js closure variables", which means they were available as "kinda globals" for any piece of code within toolbar.js. IMHO this is a bad practice. Especially if you take into account that 1) not all code within toolbar.js needs access to them, 2) the two Backbone Views in particular can then access the model via boththis.model
andmodel
. This was trivial to fix forview
, formodel
I had to move some things around.Feedback
Drupal.toolbar
" is good, but I'm wondering why we should enforce this limitedness? For the "handler" functions, wouldn't it be more elegant if we would just have a publicly available Backbone View. E.g., you could add the following to any JS file that loads after contextual.toolbar.js (e.g. tour.js):is fine though.
drupalSettings.toolbar.trayOrientationMQ = ""
instead ofdrupalSettings.toolbar.breakpoints = {}
)Drupal.toolbar.setActiveItem
does not match with how it's used in tour.module nor contextual.module. In those two modules, making the tab/toggle "active" means activating a mode that does not preclude the menu/shortcuts/user tabs from being active.ToolbarView.refreshTabs()
already deals with this correctly (but only by accident), but that's besides the point. The point is that neither of those two modules should be callingsetActiveItem()
in the first place, AFAICT.ToolbarView.refreshTabs()
andToolbarView.changeOrientation()
should be prefixed with underscores, because they're internal/helper methods. This makes it easier for others to navigate the code. Furthermore, I think_render(Tabs|Orientation)()
would be better names. In particularchangeOrientation()
is a poor name because it suggests it will make changes, whereas it's only rendering what the current model state indicates. The comments for them inrender()
can then also go away; the docs on these private functions should be sufficient.Why are we having shortcut.module (and user.module, and …) having add a toolbar.module-owned class?
There now is a discrepancy between the (sole) breakpoint in this .yml file versus the ones in
toolbar.base.css
.This was only necessary because
mql
is an object.Shouldn't this be Drupal-prefixed, i.e.
$.fn.drupalToolbarMenu
?Comment #10
jessebeach CreditAttribution: jessebeach commentedNot that they should be exposed on
Drupal.toolbar
, no. I moved them into the IIFE scope as named function declarations.The three handlers respond to events that the toolbar dispatches when its container might need to respond by resizing or positioning other page elements. So the toolbar itself subscribes to these events. Really though, I'd love it if it wasn't pushing and pulling page elements like the body around so much.
Good change. Originally we implemented this configuration using the breakpoint system. That hasn't been integrated all that well yet and I"m not sure if it gives us any advantages over just hard-coding this one value. Time will tell.
I'd like the toolbar to know what items are active, even if the rest of the items aren't automatically deactivated. This would allow us to, let's say, deactivate quick editing if the Overlay is activated (or at least put up a warning about work being lost).
Maybe rather than passing the trigger that does the activating, we can express activation through events and callbacks. I'll think about this more. Agreed that it's not ideal yet.
What about these methods as well?
Is the suggestion to put an underscore before any method of a View or Model that isn't part of that object's defined API? I can follow that pattern, otherwise I'd rather not put any underscores and just let the comments describe the methods and their purpose.
Me too, this has always been a separate todo, but never in an issue. I've added it #1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient.
An excellent point. This is so the toolbar can discover the tabs and apply behaviors through delegation. This is a bit magical though. Maybe we need the modules to explicitly register their "tab" with the toolbar through an API call. I kinda dislike the overhead, but it does mean less magic.
The CSS should be agnostic to when the trays flip from horizontal to vertical and instead be keyed off the presence of classes, which the configured breakpoint controls by determining when those classes are swapped. I'll double check this. The rest of the media queries in the CSS should be concerned with content presentation, not positioning or orientation.
Yes.
Comment #11
jessebeach CreditAttribution: jessebeach commentedMostly this is code that responds to Wim Leers' comments in #9. I want to make sure that further work continues from the same base patch.
There is one change I made to a change Wim made that I want to point out so it doesn't slip in silently.
I removed the reference to
Drupal.behaviors.toolbar.defaults
and replaced it with athis
reference. I prefer not to reach out into the global object where possible to get variable values that are defined inside the scope.Comment #12
nod_tag
Comment #13
webchickIn talking to Jesse today, it sounded like this was pretty necessary refactoring, so escalating to major.
Comment #14
jessebeach CreditAttribution: jessebeach commentedreroll.
Comment #15
jessebeach CreditAttribution: jessebeach commentedThis patch refactors the JavaScript onto Backbone. Many behavioral quirks are resolved with this refactor. For instance, under the current code, it is possible to have more than one tab be activated. By mapping the view to a state model in Backbone, only one tab can be active at a time and this issue is resolved.
Several superfluous configurations are removed in this patch.
I tried not to do too much in this patch, focusing primarily on the refactor to Backbone so that the code follows patterns established in Contextual and Edit modules.
We are addressing CSS cleanup in #1847314: Reduce the dependency on JavaScript for the toolbar to display properly.
Further Toolbar API refinements will be addressed in #1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it.
Comment #16
jessebeach CreditAttribution: jessebeach commentedSetting to needs review.
Comment #17
oresh CreditAttribution: oresh commented- Error on line 47 of toolbar.js - you didn't remove ;
- For wide screen toolbar doesn't push the content to the right ( but it used to )
- On mobile screen toolbar tray doesn't stick to the top, so there appears a white space.
- For my local build it takes about a second for script to run, and you get a fast toolbar pop, when js is executed. Can we replace it with some smoother animation ( if it won't kill the performance )
- And the toolbar switches to mobile for 285px, thought iPhone has 320 and it's more convenient to make that width for breakpoint :) And for mobile screen toolbar now stick to the bottom and there's even a larget gap. (see screenshots)
Code:
- Drupal.toolbar.setActiveItem($('.js .toolbar .bar .contextual-toolbar-tab'));
better reduce to $('.toolbar .contextual-toolbar-tab'). this is already js you don't need js class.
- for // Broadcast model changes to other modules // can you cache the $(document) variable? you use it 4 times
- for // Implements Backbone.View.prototype.initialize() // may be get rid of this.model repetition?
- var barHeight = this.$el.find('.bar').outerHeight(); change to var barHeight = this.$el.find('.bar').outerHeight(true);
I think overwise it won't work properly in IE
Everything else looks great! But it's hard to review large patches )
Comment #18
jessebeach CreditAttribution: jessebeach commentedThank you for the feedback oresh! I'm looking through your comments now.
Comment #19
jessebeach CreditAttribution: jessebeach commentedoresh, I'm fairly certain after some forensics that the issues you list in #17 were caused by me uploading the wrong version of this patch. I'm very sorry about that. I was very surprised to see so much going wrong with it!
This is the correct version.
Comment #20
oresh CreditAttribution: oresh commentedComment #21
oresh CreditAttribution: oresh commented@jessebeach, it took me some time to return and review the patch.
But now i can't apply it - probably somebody changed the js of the toolbar )
Can you please reroll for latest core changes? Thx!
Comment #22
tim.plunkettIt only doesn't apply because this line changed, it went from subtrees.id to subtrees[id]
Comment #23
jessebeach CreditAttribution: jessebeach commentedoresh, this is a reroll of #19 that takes the changed mentioned by tim.plunkett in #22 into account.
You can force git to apply as much of a patch as it can manage by passing the
--reject
option when you callgit apply
. git will create a .rej file that contains the portions of the patch that failed to apply. In this case, the failed portions were significant because this is a large refactor, so it would have been tricky to deal with the collision.Comment #24
webchickMoving back to needs review.
Comment #25
oresh CreditAttribution: oresh commentedAwesome!
Works perfectly now! Covered all issues from #17.
Comment #27
jessebeach CreditAttribution: jessebeach commented#23: toolbar-backbone-1860434-23.patch queued for re-testing.
Comment #28
droplet CreditAttribution: droplet commentedReally hard to review this patch, mixed jQuery event, Drupal behaviors and Backbone way :)
Anyway, after applied patch:
when and where use denounce ? (and why not just use underscore _.debounce)
default value set in Model.defaults ?
model.set('offsets', offsets) ???
shouldn't it a change event in Backbone.view ? (above code trigger a change event)
Why not inside View.initialize
and more..
Comment #29
jessebeach CreditAttribution: jessebeach commentedFixed this error.
Yes, this is no longer needed. The debounce was moved to
Drupal.displace
. To answer the question of why not use Underscore's debounce, We had been trying to avoid making our scripts dependent on Underscore. That was before we reached a general consensus on using Backbone, which itself requires Underscore. Now that this is generally available, I think we can consider dropping our custom displace method and using Underscore.Right :) Much better. I updated it.
Ok, that was the straightforward stuff. Now to the meatier comments.
Here's my understanding of a Backbone View. Each one is associated with an element -- its
el
property. A View controls this element and its descendants. A View should never change DOM elements outside its view, such as thebody
. Once it does, we lose the power of the View as a contained controller of data output.Your comments made me realize that I should be treating the changes to the
body
element as just another view of the Toolbar model. Rather than handling changes to the toolbar and their effects on the body through events, we can just attach a view to the body and associate this view with the same model that drives the toolbar. I've made this update in the patch and I really like this. It continues the pattern of driving visual changes from the same model source.The distinction between the View/Models declared in
Drupal.toolbar
and the code inDrupal.behaviors.toolbar
is thatDrupal.toolbar
properties are like classes whereas code inDrupal.behaviors.toolbar
represents the instances of those classes in a context. We want to keep the Views and Model definitions clean of context as much as possible so that we can write unit tests against it. That's why we shouldn't set this codein a model default, because this depends on the context of the implementation and it makes writing tests difficult. We don't really care that the locked value came from local storage -- we care that the model deals with the value correctly and that views that refer to it render correctly.
Your comments were very helpful! They gave me more clarity on how to organize the code. What do you think of my explanations? Do they make sense?
Comment #30
jessebeach CreditAttribution: jessebeach commentedAlso, we might eventually have an auralToolbarView that is responsible for implementing
Drupal.announce
to provide audio updates to model changes rather than visual. This is another reason to keep the defaults and contextual values of the models and views outside of any one View definition.Comment #31
nod_I'm all for using backbone if it makes the code better and if it doesn't increase the size too much.
I minified toolbar.js before and after the patch (so no whitespace or comments). Before patch: 4.3kb, after: 6.0kb. that's a 130% increase. I would have expected backbonned code to be smaller. I haven't looked too much into it yet. If someone can point out the reason of this increase before I get to it that'd be nice.
Comment #32
Wim Leers#31: minified size delta confirmed. When gzipped, the difference is "only" 21% though, not 30% (when not gzipped, I get 37–39% depending on the minifier, so I'd love to know what minifier you're using :)).
Let's see if we can reduce that difference.
Currently working on a deep review, hence assigning to myself.
Comment #33
Wim LeersChanges (in attached reroll)
attach()
a bit, so that all initializations happen first, and all event binding/model updating at the end.jQuery.extend({}, options)
in BB view initializations; instead explicitly pass inoptions.strings</strong>; the views don't need the <code>options.breakpoints
.Drupal.announce
are broken in many ways: when the page is loaded, the orientation is announced, while it should not, and if it's vertical upon page load, it's announced twice. The tray announcement is also broken in that it announced something liketoolbar-item--2-tray tray opened
, which is of course not very informative. I introduced aDrupal.Toolbar.AuralToolbarView
to cleanly implement this, made the#heading
property for the tray required instead of optional and am using that as the tray name instead, which is more consistent with what the screen reader user gets to hear *and* it's much easier to understand.In doing so, I'm afraid the minified code size has increased by another 500 bytes, to 6.5 KB.
Concerns
contextual.toolbar.js
,Drupal.MODULENAME.(models|views)
refers to the "classes", intoolbar.js
they currently refer to "instances". We should standardize on one.contextual.toolbar.js
.)Explanation for the increased size (even after minification)
Drupal.Toolbar.ToolbarModel
contains only 7 "attributes". If you think you can build the same functionality with *less* code, then instead of rewriting everything, you can just write more efficient Backbone views :)Comment #34
Wim LeersAs per change 4 in #33, this is probably warranted.
Comment #36
Wim LeersTest failures due to
#heading
still being assumed to be optional in the tests. Easy fix. Let's first see whether Jesse agrees with my direction :)Comment #37
jessebeach CreditAttribution: jessebeach commentedMy thinking around using extend here is it provides a way for settings to get into a View from contrib modules.
See this line in toolbar.js above the View instantiation:
The defaults and settings are merged into a new options object. If a contrib module overrides one of the View definitions, it will still get instantiated in the same way and get any new options because we're just dumping the whole object in and not plucking specific properties in the attach function.
Comment #38
Wim Leers#37: Fair enough :)
Comment #39
droplet CreditAttribution: droplet commentedmy thoughts:
- Keep all defaults value in the Backbone Model.
- Passing all of them into Model rather than Views. So, you can add more custom value and make use of BB.
- MenuView, I converted to use a Backbone.Collection. It lets you easily to extend and interact with BB , eg. reordering (collection.sort)
- move some events to ToolbarView view. This is teh main view of Toolbar, it should take over of events and let you override stuff with BB.
- minor code improvements.
I planed to drop a placeholder at beginning, and add a event handler to insert HTML code.
However seems like jQuery.once working based on element ID. I couldn't find a PHP way to change the subtress menu ID. any hint to me, thanks :)
For any purpose, I though it should be a unique ID/Class for each menu.
Furthermore changes:
- hack PHP toolbar_subtrees_jsonp and simplify JS Drupal.toolbar.setSubtrees.done
Cheers :)
Comment #40
oresh CreditAttribution: oresh commentedComment #42
jessebeach CreditAttribution: jessebeach commenteddroplet, that's for proposing these ideas! We're actively experimenting with our Backbone patterns in Drupal. I have some questions for you below.
I'd like to understand better what advantage a Collection gives us in the case of the Toolbar. We only have 2 models and they not really related -- not in the way that a Library Collection would be related to Book Models. I don't believe we need to act on model changes as a group, which is what a Collection would do for us. What use cases are you anticipating that this change would allow us to support?
One of the reasons we've been keeping the defaults in the behaviors object is our recent pattern of splitting the visual view from, let's say, aural views or keyboard views, which might need those strings as well. This patch really lays out the division of Views well #1971108-20: Convert contextual.js to use Backbone (and support dynamic contextual links).
I don't think we can change the code in this way. The subtrees are loaded through an asynchronous AJAX request. We can't guarantee that we'll have the link data when this code is executed. Using the jQuery Promise object let's us set up the framework for inserting the links once we have the data, but it doesn't matter when that data arrives. Do the changes you support here still support that asynchronous loading? Maybe I'm just not grokking it.
Comment #43
droplet CreditAttribution: droplet commentedOLD PATCH: All menu sections is one model
NEW PATCH: each menu section is a model
the menuCollection is a set of models of menu sections (Content, Structure, Appearance..etc).
- Collection is more flexible than model (http://backbonejs.org/#Collection)
Lets say when I dynamically add new menu section to toolbar via JS, and sort by "X". I can use http://backbonejs.org/#Collection-comparator directly.
I can find a menu item via http://backbonejs.org/#Collection-findWhere
More than that.
Ideally, Drupal backend should output an array of menu items rather than rendered HTML. Menu items html control by BB.render() & BB templates. smaller JSON response, easier to change the html structure
(Shortcuts-like modules may take advantage of it)
All views sharing SAME toolbarModel. I see no reason to pass it into View directly.
It's same.
Old way: Drupal.toolbar.setSubtrees.done() trigger Model change event -> Render()
New way: Drupal.toolbar.setSubtrees.done() trigger Collection.reset event -> Render().
Developers may change the render() function. Recopy the code to execute $.drupalToolbarMenu(); is meaningless.
This is the main reason I move it out.
Ideally, I'd split Render() into addOne & addAll.
addOne, dynamically insert a menu section
addAll, optimized for render all items at beginning (no reflow)
However, it's too much in CORE, so i leave as it :)
Comment #44
jessebeach CreditAttribution: jessebeach commented@droplet, I must first apologize that this patch doesn't include the changes you introduced #39. I can't yet grok the advantages of using a Collection for thesub menu items when don't really support sub-menu item fetching from the server-side. Although I think this will be a great enhancement, it strikes me as pre-optimizing right now. Plus, it took me all day to refactor what I had in #33 on the many changes to core since last we looked at this patch, so I simply ran out of time.
This patch is based on a couple refactorings to Toolbar CSS (see below for exact issue comments). I didn't want to have to adjust the CSS in this patch, since it's just about JavaScript, so I prepared those other two patches today.
I modeled the update on the JavaScript in Contextual module, which represents our best patterns to date in Drupal core JS. We're not trying to introduce any new behavior here. The goal is to rationalize the JavaScript and remove some quirks that exist in the D8 codebase today, like the incorrect highlighting of active tabs.
Based on (apply from top to bottom):
0329b221e3c812de72c76a9a9205dc53d63ded93
#1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly
#1938044-66: Prefix all toolbar classes to prevent theme clashes
Comment #45
Wim LeersI tested all permutations of actions I could think of, in combination with the Overlay module, but only in Chrome. The changes here should work cross-browser.
UX review
Found a bug: 0. do this on a laptop/desktop, 1. make viewport as wide as possible, 2. open "menu" tray, 3. toggle tray orientation to "vertical" (now
Drupal.toolbar.trayVerticalLocked = true
is set in localStorage), 4. reload the page (note that the localStorage entry remains unchanged), 5. attempt to toggle back from vertical to horizontal: the first try will fail, the second will succeed.What I also think to be a bug: when the viewport is as narrow as possible and the tray orientation *must* be vertical, the orientation toggle is still visible and allows me to switch the orientation to horizontal. When I do that, the end result looks broken. IMHO this should be forbidden.
Finally, I find it rather confusing that the "Home" tab is always pressed on the home page. If you click the "Menu" tab while you're on the homepage, it looks like two tabs are active simultaneously.
Code review
In the attached reroll: fixed a bunch of typos in the docs, made some minor changes and renamed a few functions for better consistency.
Further feedback:
I was going to say this won't work in IE9, but AFAICT it will: http://kangax.github.io/es5-compat-table/#Function.prototype.bind
This needs to be either removed or used.
s/item/tab/ ? The description does not match the variable name.
"isPaneled" hardly matches the description. Can you think of a more descriptive name?
… but AFAIK it is *impossible* to detect screen readers. So, do we instead mean "if we detect keyboard navigation"? Or, how is this planned to be done?
In any case, any remaining @todos must have issues for them.
This is all rather puzzling at first sight: why would the toolbar need to touch the body element?
I think we must very explicitly document here that the overall architecture for the toolbar's CSS requires these classes to be set on the body element. It should possibly here, and otherwise elsewhere, also explain why we let the JS handle media queries rather than the CSS.
Based on (apply from top to bottom):
37e0bf87ed682b4de3b8570416d98c226c912a87
#1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly
#1938044-66: Prefix all toolbar classes to prevent theme clashes
Comment #46
falcon03 CreditAttribution: falcon03 commentedI was going to test this patch, but it doesn't apply! :(
In the last few days, I've noticed something weird with the toolbar: after expanding an item (let's say "menu"), the drupal.announce works as expected, but Voiceover does not intercept the element that appears in the page unless you reload it. I'm not sure that this is not a bug in my setup, tough. I'm using Mac OS X 10.8.3+Safari.
@Wim Leers, @Jessebeach, what do you think about the bug I mentioned? Could it be related to the toolbar JS? If so, will it be fixed in this issue or should I file another (at that point critical) bug report against the toolbar module?
Comment #47
jessebeach CreditAttribution: jessebeach commentedPostponing on the commit of #1938044: Prefix all toolbar classes to prevent theme clashes.
Comment #48
jessebeach CreditAttribution: jessebeach commentedRerolled and addressed input from Wim Leers in comment #45.
Fixed this by introducing a validation step to the call to set for orientation. This was happening because the orientation was getting set to horizontal before it was checked for being locked in a vertical position. Using validate() before set() can occur cleans up the code a bit more.
I'm inclined to leave this because (1) it's not destructive and easily reversible and (2) some trays with very little content might display well on a narrow screen in a horizontal orientation.
Yes, me too. But I don't want to solve that there. There are several design issues opened against the Toolbar. We're going to end up addressing this point before Drupal 8 ships for sure.
Function.prototype.bind
is defined in IE9 as far as IE claims.I changed
isPaneled
toisFixed
.Right, this @todo ends up being outdated with the introduction of Promise and the done handler. We do have an issue for this already: #1927174: Complete the work to make the Toolbar administration menu sub tree rendering more efficient.
Right, lots of comments added. We're using JS to handle media-query handling for two reasons: (1) Using JS let's us leverage the breakpoint configurations and (2) the CSS is really complex if we try to hide some styling from browsers that don't understand media queries. If we drive the CSS from JS, the CSS becomes more robust.
Based on (apply from top to bottom):
e0f675f3b24735df93b03a6983aa7cfc23235322
#1847314-37: Reduce the dependency on JavaScript for the toolbar to display properly
#1938044-72: Prefix all toolbar classes to prevent theme clashes
Comment #49
Wim Leers#48 addresses all questions I had in #45.
Only one point I disagree with:
Your points are right, but you omit one big negative point: the fact that in most cases, it looks terribly broken. IMHO it's very simple: when the media query makes the tray default to vertical orientation, then also remove the toggle completely.
Better to prevent "WTF THIS LOOKS WEIRD AND KINDA BROKEN!?!!" than to allow for a feature that would probably be used only very rarely.
This refers to Backbone 1.0, but we're using Backbone 0.9.2 in current D8.
Is this future proofing?
Also: that's a whole bunch of code. Why not just do
set(attributes, {validate:true})
to avoid all this?I see the additional comments, but they lack the clarity that this explanation has in #48. Could you maybe make the docs in the JS read more like this explanation?
I again tested this manually, and it's still working great. The bug I found in #45 has indeed been fixed in #48.
Comment #50
jessebeach CreditAttribution: jessebeach commentedYes, we discovered in the backporting of D8 features to D7 (where we're already using Backbone 1.0), calls to set() do not trigger validate unless
{validate: true}
is passed (I know Wim knows this already :) Just elucidating for folks who review this issue). There are three places in the code where I need to induce validation, so I removed the set() method override and just pass in validate option in those cases.The toggle is now hidden below the wide breakpoint and available above the wide breakpoint. This prevents the user from clicking themselves into a situation of ugliness.
I also changed instances of
.find('> .something')
to.children('.something')
.Comment #51
Wim LeersI think this is RTBC now :)
I can't RTBC until #1938044: Prefix all toolbar classes to prevent theme clashes is committed though, until it is committed, this issue needs to continue to be marked as postponed.
Comment #52
Gábor Hojtsy#1938044: Prefix all toolbar classes to prevent theme clashes landed. Sending to testbot.
Comment #53
Wim LeersThis hunk will make the patch no longer apply to HEAD.
Fix that, and this will be RTBC.
Comment #54
Wim LeersMy bad #53 was a hunk that only existed in the interdiff; it is not in the patch itself. Which means the patch should work just fine. RTBC as per #51.
Comment #56
Gábor Hojtsy#50: toolbar-backbone-1860434-50.patch queued for re-testing.
Comment #58
Wim LeersThese look like legitimate test exceptions. Jesse, can you reroll?
Comment #59
markhalliwellAppeared that the toolbar_test module wasn't setting
#header
which was causing the tests to throw an exception. Went ahead and refactored that theme function to check if#header
isset too though.Comment #61
markhalliwellwh00ps! Didn't mean to change the logic :)
Comment #62
markhalliwellInterdiff for #59 -> #61, sorry using drush_iq... it happens sometimes :)
Comment #63
markhalliwellHere's the interdiff.txt for #50 -> #61.
Comment #64
jessebeach CreditAttribution: jessebeach commentedI made a slight improvement to the way that displacement is calculated on the initial load of the page, so that the when a horizontal tray is presented, the padding on the top of the body will always be sufficient to push the page content down. There's was a race-condition for this calculation that is eliminated with a call to
setTimeout
.The interdiff includes markcarver's test change because it was just easier to roll it that way because of the order that I applied his fix to my dev branch.
Comment #65
markhalliwellDelay milliseconds are required on setTimeout(), (not sure about newer browsers/js engines, but for BC this should probably be set to a minimum of 10).
Comment #66
jessebeach CreditAttribution: jessebeach commentedDERP! Great catch markcarver. In this case, a delay time of zero will be sufficient to put the invocation of the callback function into the next execution cycle.
I just made one change in this patch from #64.
We have an issue to do one more pass at accessibility: #1800614: Improve the responsive toolbar accessibility
I'd like to keep these issues separate, since this issue gets us to a great, improved JavaScript base on which to build the Views to improve the Aural experience of the Toolbar. The Spark team has shown commitment to making these improvements. I can personally promise that I will get these accessibility improvements in during this release cycle.
Comment #67
markhalliwellReviewed with Dreditor, looks good. Nothing standouts out to me, patch passed :)
Comment #68
markhalliwellRemoving tag since #1800614: Improve the responsive toolbar accessibility exists.
Comment #69
dsnopekDid some quick manual testing. Everything seems to be working fine!
Comment #70
droplet CreditAttribution: droplet commentedQuick reviews:
Can we remove "body." ?
What is it?
`if($tab.length)` is fine ?
Access DOM directly ? $tab[0].id
Not sure if it is essential. It sets an empty value.
Comment #71
Wim Leers#70: please don't do "just stopping by for a quick review" reviews on patches that are already RTBC, especially if you're not even bothering to understand what's going on.
RE: "body": this is for good reason, see the comments above.
RE: "$tray": that's for defaulting to an empty jQuery set, it gets filled later on in that function.
RE: "length > 0": besides your suggestion containing a coding standard violation, using "length > 0" is fine: it is more explicit.
RE: "$tab[0] rather than $tab.get(0)": they're identical…
RE: "empty data-offset-top": this is for good reason, see elsewhere in the patch where "data-offset-top" appears.
These sorts of reviews are useful when a patch has not yet been vetted, but at this point in the lifecycle of a patch, these superficial reviews are most unhelpful.
Comment #72
jessebeach CreditAttribution: jessebeach commented#66: toolbar-backbone-1860434-66.patch queued for re-testing.
Comment #73
Dries CreditAttribution: Dries commentedThis looks good but can we please document the API changes in the issue summary per https://drupal.org/node/2045345. Given that we are past API freeze, API changes need to be approved and documented.
If another Javascript maintainer could review this, that would be great, but not necessary.
Comment #74
nod_I reviewed a bunch of times, I'm happy with the code overall.
I'd agree with droplet that tray = $() isn't needed here. It was relevant in previous versions of the code but now the patch sets $tray = this.$el.find('[data-toolbar-tray="' + name + '"].toolbar-tray'); in all cases so the initialisation is not needed.
for .length and .get(0), it's a little different from the rest of core but it's valid use of jQuery API so no big deal.
for the data-offset-top, Drupal.displace uses the fact that the attribute exists and figure out itself the height of the element so even an empty attibute is used. I'm not really found of the behavior but it was discussed in another issue.
maybe another minor nitpick:
it ought to be an if/else instead of a switch.
RTBC +1
Comment #75
xjmThanks @nod_! Do you think the minor cleanups of #74 merit a followup?
We just need the API changes documented, then.
Comment #76
nod_It's either in this patch or we forget about it.
Comment #76.0
nod_added some use case notes.
Comment #76.1
jessebeach CreditAttribution: jessebeach commentedchange the api heading content
Comment #77
markhalliwellIssue summary was updated by @jessebeach, no API changes.
Comment #78
alexpottIf this is unnecessary as suggested by #70 and #74 let's remove it... extra lines of code need to maintained and in this case delivered to the client...
Comment #79
markhalliwellAlright. I went ahead and removed it and initalized it farther down. TBH it really doesn't matter and this is some serious over-nitpicking. The amount of "client performance" saved by this, is extremely negligible. This issue has been set as RTBC several times now with minor (uncessary) nitpicks, setting back to RTBC. As far as the if/else/switch issues, it's fine the way it is (more than two and less than 10 use switch: http://oreilly.com/server-administration/excerpts/even-faster-websites/w...). No need to change it.
Comment #80
alexpottCommitted f6ac79e and pushed to 8.x. Thanks!
Comment #81
hass CreditAttribution: hass commented+ opened: Drupal.t('opened'),
+ closed: Drupal.t('closed'),
Need to be Ucfirst per #421118: [Meta] Standardize capitalization on actions and consistency.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedBut "opened" and "closed" aren't actions, they're states. Here's the code that uses those strings:
In other words "Foo tray opened." and "Foo tray closed.".
Comment #83
hass CreditAttribution: hass commentedDo not be so nitpicking, please. It's not really about actions only. We are changing all lcfirst strings. :-)
That's not the point. A string
Foo tray opened
is ucfirst and therefore correct. A string likefoo tray opened
needs work, too.Comment #84
markhalliwell@effulgentsia is correct. #421118: [Meta] Standardize capitalization on actions is about single worded "actions" or "operations" (commonly found in tables), this is a completely different scenario and is located towards the end of a string where it shouldn't be ucfirst. If you really feel this should be done differently, we can can come up with a different solution in a separate issue. This was already committed in #80.Sigh, I retract my statement, @nod_ pointed out that this is only used for accessibility (screen-readers) so it really doesn't matter if it's capitalized in the middle. Created new issue to follow up this "bug": #2052973: Improve translatability of strings in toolbar.js.
Comment #85
hass CreditAttribution: hass commentedI don't like to translate both and this is why we need to be consistent.
Comment #86
jessebeach CreditAttribution: jessebeach commentedRemoving the
sprint
tag.Comment #87
Wim Leers.
Comment #88.0
(not verified) CreditAttribution: commentedupdated the summary
Comment #89
mgifford