Problem/Motivation
The initial commit of the responsive toolbar patch included a basic definition of the array structure to be returned by modules that implement hook_toolbar
.
$items[$group_name] = array(
'tab' => array(
'title' => t(''),
'href' => '',
'attributes' => array(
'class' => array('icon', 'icon-menu'),
),
),
'tray' => array(
'#heading' => t(''),
'content' => array(/* renderable content */),
'weight' => -15,
);
The structure returned by modules implementing hook_toolbar
is needed to produce:
- A top-level tab.
- An optional tray of links or control structures. The content of the tray is arbitrary.
Tabs are returned as link structures and merge into an array passed to theme_links
. theme_links
passes each link to the l() function or directly renders the item with a span. This results in the tab items not running through the full drupal_render process. Pre and post render functions cannot be associated with the tabs for example. The edit module, in particular, needs to determine if its tab should be rendered in a post render function.
We need to determine an array structure to be returned from hook_toolbar
that is flexible and simple to implement.
Use Cases
Countcount module
The Countcount module wants to have a drop-down list in the Tab area: Zero, One, Many. So far, so good: my patch allows any render element to be used for a tab. Next, when the user clicks on "Many", the corresponding tray should open, so that the user can select the actual number.
User module
Show the avatar image of an authenticated user in the User tab.
ShoppingCart module
Show the number of items a user has in their shopping cart in a toolbar tab. Clicking the tab brings the user to the shopping cart management page. The ShoppingCart module maintainers also want to provide a "drop button" like menu with the option "Check out" on the toolbar tab. This should not be a toolbar tray.
MessageStream module
A hypothetical MessageStream module wants to place a button in the toolbar that opens a sidebar with a list of system messages. The placement and styling of the button should not be like a toolbar tab. In fact, all styling and behavior will be custom. In this case, the MessageStream module is using the toolbar as a simple container.
Search module
Proposed resolution
The purpose of hook_toolbar
is to provide module developers a means to add components to the toolbar. The toolbar is meant to be a container for top-level site actions and configuration. See the use cases above for examples of what a module might add to the toolbar.
In toolbar also provides a construct known as a "tray". A tray is a container that will display vertically on narrow screens and can be display horizontally on wide screens. A module developer can specify renderable content that should be placed in a tray. The tray is controlled by a trigger, normally an HTML link element.
jessebeach and benjifisher have been working through an approach to improve the processing of hook_toolbar
so that developers can leverage the toolbar tray system, but also be able to pass any renderable element as a component of the toolbar.
API changes
hook_toolbar()
- An implementation of
hook_toolbar()
should return a keyed array of render elements, each element of typetoolbar_item
.
- New render elements
- We define a new type of render element,
toolbar_item
, with properties'#tab'
(required),'#tray'
, and'#weight'
. The tab and tray can be any render elements, and the weight is an integer. This type is given a default pre-render function,toolbar_pre_render_item()
, theme function,theme_toolbar_item()
, and theme wrapper,theme_toolbar_tab_wrapper()
. We also define a newtoolbar
type, which is used internally. If the tray is present and javascript is enabled, then the tab will toggle the tray's visibility.
- Theme functions
-
theme_toolbar()
: Responsible for theming the toolbar and its child components.theme_toolbar_item()
: Responsible for theming the toolbar_item element. This renders only the tab, leaving the tray fortheme_toolbar()
.theme_toolbar_tab_wrapper()
: Wraps the#children
of an element in a div with the classtab
. Provide common styling for "tab" elements in the toolbar.theme_tray_heading_wrapper()
: Appends an<h3>
heading with the contents of a#heading
property to a tray. Used for accessibility landmarking.theme_toolbar_tray_wrapper()
: Wraps the#children
of a rendered content of a toolbar component'stray
property in markup that styles and provides behavior attachment for a toolbar tray.
Some of the follow-up issues may add default properties of the toolbar_item type and/or change the list of theme functions, but we have settled on the '#tab'
, '#tray'
, and '#weight'
properties of the toolbar_item type.
Example
Under this patch, a module might return the following renderable in hook_toolbar()
. This code would create a tab in the toolbar with the label "Shopping cart" and an associated tray with actions associated with the cart.
$items['commerce'] = array(
'#type' => 'toolbar_item',
'#tab' => array(
'#type' => 'link',
'#title' => t('Shopping cart'),
'#href' => '/cart',
'#options' => array(
'attributes' => array(
'title' => t('Shopping cart'),
),
),
),
'#tray' => array(
'#heading' => t('Shopping cart actions'),
'shopping_cart' => array(
'#theme' => 'item_list',
'#items' => array( /* An item list renderable array */ ),
),
),
'#weight' => 150,
);
Remaining tasks
- Gather feedback and opinions on the proposed patch in #58.
- Manual testing.
User interface changes
None most likely.
Original report by jessebeach
This issue is a followup to: #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop
Related issues.
#1869638: Make the menu shown in the administration menu tray configurable
#1877468: Finalize usage of hook_toolbar_alter()
#1877482: Toolbar tabs should have ID attributes based on hook_toolbar() array keys rather than sequentially numbering
#1877486: Clean up code in toolbar.module
Comment | File | Size | Author |
---|---|---|---|
#74 | toolbar-refactor-1847198-74.patch | 6.59 KB | effulgentsia |
#71 | toolbar-refactor-1847198-71.patch | 36.98 KB | effulgentsia |
#71 | interdiff.txt | 654 bytes | effulgentsia |
#70 | toolbar-refactor-1847198-69.patch | 38.29 KB | jessebeach |
#70 | interdiff_68-to-69.txt | 10.81 KB | jessebeach |
Comments
Comment #1
jessebeach CreditAttribution: jessebeach commentedadding an image
Comment #1.0
jessebeach CreditAttribution: jessebeach commentedadded more info
Comment #2
webchickThe thing I like about toolbar_theme_tabs() and toolbar_theme_trays() is that it ideally helps address sun's feedback in the original toolbar issue about being able to put things other than links in this place. The Northstar designs even have a Search module widget in the "tabs" section, and it's also the ideal place to put contextual controls like the "Save / Publish" button when inline editing, or the mobile preview functionality from Spark.
Comment #3
Shyamala CreditAttribution: Shyamala commentedComment #4
benjifisherShouldn't it be
theme_toolbar_tabs()
(or_tab()
) andtheme_toolbar_tray()
instead oftoolbar_theme_tabs()
andtoolbar_theme_tray()
?On second thought, I do not think we want to use theme functions for this. That would be useful if we wanted to let themers give a custom design to all the tabs, but I think what we really need is to allow the module that defines the tab/tray combination to specify how it will be rendered.
Perhaps the simplest thing is to let each tab and each tray be an arbitrary render element. The module that defines them can specify any standard or custom #type. Or maybe #theme. By default, we will use
'#type' => 'link',
for the tab elements, so they will be rendered bytheme('link')
. This will require minor changes to the existing implementations, becausetheme_link()
andtheme_links()
take different keys. (argh!)If people like this approach, I think I can implement it. All I need is some time. I would like to know what @sun thinks.
@webchick, can you give a pointer to the Northstar designs? I am clueless.
Comment #5
jessebeach CreditAttribution: jessebeach commentedbenjifisher, adding #types toolbar_tab and toolbar_tray sounds promising. If you could post a simple initial patch we can hash through it.
Comment #6
benjifisher@jessebeach:
Can do, but not today. (I am not sure we will need the types you suggest, but I will have to think about it.)
I spent all morning adding issues (several screen shots, two tiny patches). It is a relief to get all of these questions out of the back of my mind and onto the issue queue, but it used up a lot of my available time.
Comment #7
benjifisherI have only had an hour or two to work on this. The attached patch is a work in progress. In other words, it totally breaks the toolbar. I am not sure that I have the right keys (
'text'
or'#text'
?) for my render elements, for example. If I am lucky, a rewrite oftemplate_preprocess_toolbar()
will get it close to working again.Once the code is working, we will have to rewrite the tests and toolbar.api.php.
Comment #8
benjifisherUpon further investigation, I see that an element with
'#type' => 'link'
is processed bydrupal_pre_render_link()
and nottheme_link()
. So instead of'#text'
and'#path'
I should use the keys'#title'
and'#href'
.The attached patch is still a WIP, but it is a lot closer. I am getting errors from
drupal_process_attached()
, and I am getting unstyled lists of links, but at least I have the attributes about right. Now I think it is time to look attemplate_preprocess_toolbar()
.Comment #9
benjifisherI have a simple use case in mind. The Countcount module wants to have a drop-down list in the Tab area: Zero, One, Many. So far, so good: my patch allows any render element to be used for a tab. Next, when the user clicks on "Many", the corresponding tray should open, so that the user can select the actual number.
First: is this the sort of use case we want to support?
Second: if so, then how do we structure it? What should
hook_toolbar()
return to indicate the correspondence between links and trays, keeping in mind that we want to default to a single tab owning its own tray.Comment #10
benjifisherI have one possible answer to the question I asked in #9. (Please, stop me from talking to myself!)
As long as
toolbar_view()
builds a render array, the Countcount module can pass the Many link incountcount_toolbar()
and then add the Zero and One links incountcount_toolbar_alter()
.I think this approach makes good use of the structure we already have, and it keeps the structure simple for modules that add a single link to the Tabs section.
Comment #11
jessebeach CreditAttribution: jessebeach commentedThe association between the tray and tab has to be soft (i.e. not expressed through DOM relationships) because of styling and placement. Also, I'd like the Toolbar to have some kind of common handling for opening a tray when a tab is pressed.
I'll look over the patch in #8 toady.
Regarding the use case, I think it's interesting. Let's put it and a few more in the description of the issue and see what type of behavior we might want to support.
Comment #11.0
jessebeach CreditAttribution: jessebeach commentedadding more info
Comment #12
jessebeach CreditAttribution: jessebeach commentedAdding some images for the issue summary.
Comment #12.0
jessebeach CreditAttribution: jessebeach commentedadding use cases
Comment #13
benjifisher@jessebeach:
Thanks, you saved me from talking to myself!
Unless I am missing something, my suggested use of
hook_toolbar_alter()
in #10 is consistent with your point about "soft associations" in #11. We would have to rearrange the code intoolbar_view()
so that the sequence ishook_toolbar()
hook_toolbar_alter()
In the Countcount use case, this means that the contrib module does not have to know what is going on in Step 2. Because the Many link has already been processed, it has the necessary "soft association" to its tray. Now the contrib module can alter the render array, using the existing Many element and adding the Zero and One links.
I will have to think about the other use cases. Thanks for providing them.
Comment #14
benjifisherFinally, a patch that is not completely broken! The status is still NR, for a variety of reasons: need for clean-up, need to update tests and .api.php, maybe others. Oh, yes, I have to restore the
<h2 class="element-invisible">Toolbar</h2>
.It turns out that the main problem with the patch in #8 is that I tried to "clean up" the line
by removing the "extra"
'toolbar'
. Live and learn!I think the code is good enough that I want to continue the discussion in #9—#13 before I do any more.
Comment #15
benjifisherBack to talking to myself ...
I think I like the approach I outlined in #13. I will have to rearrange the code a bit, deferring the sort until after invoking
hook_toolbar_alter()
, because I want to make it easy for other modules to change the weights.Overall, there is nothing in Drupal that cannot be represented by a renderable array, so I think that making the tabs an item_list of render arrays is flexible enough for all the use cases that Jesse added to the issue summary.
I want to get into specifics of
toolbar_view()
.I added the
array_map()
line above, replacing the ones below, which seem to have been added back. I am pretty sure they have the same effect.I thought that
array_map()
might be more efficient thatforeach()
, and PHP 5.3 allows us to use anonymous functions for the callback. If the second form is easier to understand, or I guessed wrong about efficiency, then we can stick with theforeach()
I think I am responsible for these lines:
It looks like an overabundance of caution. Is there any way the condition can be satisfied?
Comment #16
benjifisherThe attached patch works well enough that I have started to clean up the code and add comments. It would be interesting at this point to add a module to the Examples project that implements one of the use cases we have described. See whether this patch actually does what I think it does!
One little annoyance. I was not sure of the right way to get the second line of this markup:
The problem is that
theme_links()
handles a heading, buttheme_item_list()
hard-codes its title as an<h3>
. I solved this with a few lines intoolbar_pre_render()
and the template file. It makes sense to me, but maybe there is a better way.edit: D'oh! I left in a
dsm()
line.Comment #17
benjifisherI modified toolbar_test.module to be consistent with the new
hook_toolbar()
. The test passes locally, and I am setting the issue to NR so that the test bot can have its say.Comment #18
penyaskitoSome nitpicking:
Do we really need t() here? I'm not sure about it.
There shouldn't be a @todo in the code, but a follow-up issue or a fix.
Should be ARIA instead of aria?
Commented code should be removed.
Very great job BTW. And I am really learning from your "self-talks", thanks for taking time for documenting your way to understanding too :-)
Comment #19
benjifisher@penyaskito:
Thanks for the review! Maybe it is too early to set the status to NR; I will set it back to NW. I certainly want to make further changes before the code is committed.
I consider "talking to myself" to be part of the documentation as well as an invitation for feedback. I am glad you find it helpful.
t()
. This is text that will be displayed on the site.array_map()
orforeach()
.Before the code is committed, we need a module that tests it. I am working on that, now. That should serve as a basis for testing this patch, updating toolbar.api.php, and updating the tests. When all that is done, I will try to add the module to the Examples project.
Comment #20
benjifisherI have set up a sandbox project for a testing/example module for the toolbar hooks: http://drupal.org/sandbox/benji/1862108.
I would like to have help on the sandbox project from someone who knows CSS, icons, and javascript better than I do. For example, I implemented the Countcount example I described in #9 above. I am happy to report that it works as I hoped it would, but I do not plan to refine it. It could use some love in these three areas. See the screen shot: the "0, 1, 2, Many" should be a drop-down list, and the form could be prettier.
One thing I noticed is that the PHP sorting functions are not stable. If several tabs have the same weight (or none, which is equivalent to having weight 0) then they are sorted unpredictably. I think this is a problem. What is the best solution? My initial thought is to sort by link title (case insensitive).
Comment #21
jessebeach CreditAttribution: jessebeach commentedSorry for the delay in responding. Let me go through the work you've done and how I extended it.
Using array_map is much cleaner. I removed the foreach code.
I'm not sure that we need this h2 from an accessibility standpoint. Not having it didn't come up as an issue when we did a11y reviews in Toronto. I've removed it for now.
Now, to the changes I've introduced. I really wanted to get away from called
theme_links
on the tabs. If we're going to put content in the bar like a search bar or a right-aligned tab, we need to give developers a way to opt out of creating a visually-styled tab and also a tab that controls a tray. Also, the tray cannot be necessary. To that end, I introduced a theme function for the tray toggle (a linke that controls a tray) and two theme wrapper functions to create a visual tab and a tray. None of these theme functions are necessary.This is how the User module implements the theme functions.
When we preserve the renderable nature of the content, we can leverage the theme layer to sort the tabs, thus I changed the
weight
property to#weight
and removed the ua sort fromtoolbar_view
. This function becomes much less complicated as well.The bulk of the tray and tab processing moves to
template_preprocess_toolbar
where users can alter the output with more ease.The tray toggle link attributes move to
template_preprocess_toolbar_tray_toggle
, where again, they can be altered before render.I tested the changes with Edit module #1824500: In-place editing for Fields in a followup issue that will be blocked on this one #1860436: Remove toolbar-specific code from the Edit module. The changes proposed in this patch work splendidly.
I also added a search box with a
#weight
of 150 to the bar and it came through without a problem. So the search module could theoretically implementhook_toolbar
and put a global search on the page.benjifisher, are these changes at all in line with your Countcount use case? Hopefully the changes to Shortcut and User provide sufficient example.
Comment #22
benjifisherWow, that is a lot to respond to. I will start with the easy stuff.
I am glad we agree on
array_map()
vs.foreach()
.In addition to removing the
<h2>
from the template file, you have removed all the<h3>
elements. I am not sure whether they belong in the template file or elsewhere, but I like the idea of keeping all the h2's and h3's in the same place. I defer to others on a11y issues.Looking through the interdiff, I noticed this line:
I think it is simpler, and probably more efficient, to use
Either way, I am not sure we need the second condition. Is there a chance that the key is there, but
$item['#tray']
is set toNULL
?So much for the easy stuff.
The interdiff is larger than the regular patch. I think we have two very different approaches.
I completely agree. My first patch on this issue includes
As I have said before, I think that allowing a tab to be any render element gives modules complete flexibility. I may think differently after studying the patch in #21, but my initial impression is that, using it, module writers would have to learn the toolbar-specific elements and theme functions instead of using generic render elements.
The main work in going from the patch in #14 to the one in #16 was to implement the idea outlined in #13: do some processing after invoking
hook_toolbar()
and before invokinghook_toolbar_alter()
. The patch in #21 goes back toI do not see how I can get the Countcount example to work using this structure.
Let's compare how a module can add a search field to the toolbar using the two patches. Based on the patch in #17, the toolbar_example module (screen shot in #20, code in my sandbox http://drupal.org/sandbox/benji/1862108) includes these lines:
(Of course, those lines also add a form to the tray for a different tab.) @jessebeach, can you post the code you mentioned in #21 for comparison?
I do not think we want to defer sorting to the theme layer: see my remark in #20. OTOH, the decision to use
'weight'
or'#weight'
is an important one, not just whether to usedrupal_sort_weight()
orelement_sort()
. It is really a conceptual question of whether our array is a render element (with'#weight'
as one of its keys) or a more generic array. I think we should use render elements at some level, but I have been wrestling with the question of whether we should have one big element with keys'#tab'
,'#tray'
, and'#weight'
or whether'tab'
and'tray'
should be children. This decision affects how default values (defined inhook_element_info()
) are merged in and the theme functions that we are exected to provide. I think I need a render guru.Comment #23
benjifisherGet rid of the trailing space in the title ...
I think that render arrays should be modified in preprocess functions, not in theme functions, as in this (from the patch in #21)
This is too late in the process for the Countcount strategy that I outlined in #13. I guess my module would have to add a preprocess function instead of implementing
hook_toolbar_alter()
if we use the approach in #21.In general, adding theme functions for better separation of markup and processing is a good idea. We can easily add that structure on top of the patch in #17. But I do not like the idea of requiring the modules implementing
hook_toolbar()
to add these theme functions via the'#theme'
key. That should happen intoolbar_view()
(before invokinghook_toolbar_alter()
) or in the preprocess function. Cf. my remarks in #22 about render elements.One peculiarity of the approach used in #17 is how one module can add a tray to another's tab (or vice versa). This is probably an edge case anyway. It can be done using
hook_toolbar()
(sincemodule_invoke_all()
combined the results from different modules usingarray_merge_recursive()
) but not inhook_toolbar_alter()
(since that is after the processing that adds the connection between tab and tray).Comment #24
benjifisherFollow-up to my last point in #22:
After sleeping on it, I have decided that the tab and tray components of the array returned by
hook_toolbar()
should be properties, not children. The point is that we are looking for specific keys, and children can have any keys the module developer chooses.So I think we should define one new element type: 'toolbar', or maybe 'toolbar_item' or maybe something involving 'tab' or 'bar'. A module that implements
hook_toolbar()
should return a keyed array of these elements. The general structure should beWe can also add some other options in '#options'; I am not sure what the use cases are. My initial thought is that extra '#theme' or '#theme_wrappers' can be added to the child elements, but maybe this is a better place to specify them.
The patch in #21 give the module developer the responsibility for adding the appropriate theme wrapper if a tab is meant to activate a tray (rather than be a simple link). The patch in #17 makes that decision based on whether a tray is defined as part of the same element, and a module can override that decision using
hook_toolbar_alter()
. I prefer the approach that makes the module developer's job easier.I have also been thinking that we might go back to @jessebeach's earlier design of allowing each tab/tray combination have its own flag (or set of breakpoints) for switching between horizontal and vertical. At narrow screen widths, I find that the long admin menu is best viewed vertically, but trays with only one or two items leave a lot of white space. This could be one use of '#options'.
Comment #25
jessebeach CreditAttribution: jessebeach commentedGreat thoughts in #23 and #24. First, I moved much of the logic in
theme_toolbar_tray_wrapper
to a preprocess function. See 1847198_toolbar-dx-preprocess-fn_21_do-not-test.patch. Good call.I changed the tray headings back to
<h3>
.The big change in this patch is moving tab render arrays under the #tab key. I'm intrigued at the notion, although I don't think I've gotten it right yet. Mostly, it ends up creating an awkward processing block in
toolbar_preprocess_toolbar
Where a developer could include renderable content in the returned item and in
#tab
and both would end up making tabs. The use-case I'm trying to solve is represented in the included search module patch, which sits on top of the proposed patch in this issue.I think what needs to happen is the user needs to pass
'#theme' => 'toolbar_tab'
with a#tab
property and optional#tray
property. Otherwise, we just pass the contents of the item passed to hook_toolbar to the whims of the render layer. I'm going to see what that looks like in a followup.Comment #26
jessebeach CreditAttribution: jessebeach commentedI totally horked up those patches in #25! Redoing and properly basing on 8.x this time.
Comment #27
jessebeach CreditAttribution: jessebeach commentedI played around with the idea of a
theme_toolbar_item
function and I ultimately ran into this problem: If we pass the processing of #tray and #tab content to a theme function belowtheme_toolbar
, then we can conglomerate the "tabs" into the administration bar and the "trays" outside of that bar. For positioning reasons, the trays need to live outside the administration bar (the .bar element) in the DOM. The triage of elements in the bar (tabs) and trays needs to take place intoolbar_preprocess_toolbar
.Given that, I kinda like where I ended up in #26. It allows us to create the following type of content.
This patch is based on #21, ignoring #25.
Comment #28
benjifisher@jessebeach:
Thanks for the extensive examples in #27. I think there are still two major differences between your approach and mine. The differences depend on each other.
First, my idea is to make both the tab and the tray renderable arrays. So I would modify your last example as
Maybe the
'#theme_wrappers'
property should also belong to the '#tab' element; I am not sure.Which is better? If we use your method, then we cannot use
'#options'
for anything toolbar-related, since too many other elements want to process this property. I guess my real objection to your approach is that you are mixing levels. The tab element should logically be nested (as I see it) with its own element type (link, form, search). It should also have its own properties, separated from those of the parent element. Off hand, I cannot think of any more functional distinction between the two structures.The second, related difference in approach is that my patch in #17 does some processing between
hook_toolbar()
andhook_toolbar_alter()
. That allows me to implement the Countcount example (screen shot in #20) with a few lines inhook_toolbar_alter()
;(This builds on the simple definitions in
hook_toolbar()
. The full code is at http://drupalcode.org/sandbox/benji/1862108.git/blob/refs/heads/8.x-1.x:....) Using your approach, I think that the Countcount module would have to work much harder to make its fourth link the one that opens the tray.Comment #29
jessebeach CreditAttribution: jessebeach commentedThe content of the
user_messages
array is a renderable array, right? Just making sure I'm not off base here. We don't need to bury it in a#tab
key.The
#tray
is supplemental to the main renderable content ofuser_messages
in this example. If we bury renderable content under#tab
then we get ourselves into a situation where we might have two tabs for every item passed back tohook_toolbar
: (1) the renderable content in the item array and (2) the renderable content inside of#tab
. Does that make sense? I should be able to pass the following back tohook_toolbar
and have it render.Comment #30
benjifisherMaybe this is a better way to put my position: in your example
$items['user_messages']
, you have assigned properties'#text'
,'#path'
, and'#options'
. You have not assigned'#type'
, but you want those three properties to be interpreted as if'#type' => 'link'
, so it looks to me as if you are using a link element. Yes, you can assign'#theme'
and'#theme_wrapper'
properties, but it seems to me that people will understand the code better if we use recognizable element types. (Maybe the existence of a'#type'
property is what distinguishes "an element" from "renderable content"?)OTOH, if you explicitly assign
'#type' => 'link'
(or assign it by default during processing), then you are in the unusual situation of using a link element with the additional property of'#tray'
, which is ignored by theme_link. Not to mention'#weight'
.You refer to my suggestion as "burying" the renderable content. I don't know, is there any rule against making a render element the value of an element property? Is there precedent?
To repeat a couple of things I said before:
Comment #31
jessebeach CreditAttribution: jessebeach commentedAha! I think we're on the same page now!
Ok, so in my example of
$items['user_messages']
, I'M TOTALLY MISSING'#theme' => 'link'
. MY BAD. Oy, very sorry for the confusion, it should beThen, on to
I see your point here. Let me see if I can rustle up some opinions on this issue here at work.
Comment #32
jessebeach CreditAttribution: jessebeach commentedbenjifisher, I chatted with moshe about using
hook_element_info
, which you've been alluding to. I'm going to explore that option and we can hack on that a bit as an alternative?Comment #33
benjifisherjessebeach:
I really wish @sun would chime in. He is the one who seems to care most about being able to use the structure here to modify or extend or apply the toolbar.
I think you are more likely to find a render guru among your coworkers than I am.
IIUC, the point of using
hook_element_info()
would be to "register" a new element type. (As I said before, it also affects how default values are merged in and makes us responsible for defining a theme function.)There is also the issue of the Countcount example. I admit it is contrived, but I have a hunch that someone will come up with a compelling use case that follows the same pattern. I want a "complex" render array (in this example, a list of four links) and somewhere inside it is a link that activates the tray. In the patch for #27, the
'#toolbar_identifier'
property is added intemplate_preprocess_toolbar()
. I think the author of the Countcount module would have to add a preprocess function to move that property around, and this would require knowing the internals of the Toolbar module. Contrast that with my code snippet in #28, usinghook_toolbar_alter()
.I think this is a harder thing to change than whether the tab element is nested inside a
'#tab'
property.Comment #34
benjifisherI have been wondering about the comment in the documentation block
I finally checked the commit message and the corresponding issue: #612974: Optimize toolbar to only build if it will be displayed. I think we should add a comment to toolbar.api.php to the effect that the toolbar is built after
hook_page_alter()
is invoked. If a module wants to add elements to the toolbar or change its '#access' property, the module can usehook_page_alter()
, but this hook cannot be used to modify the completed toolbar.I still do not see why we need a separate function
toolbar_view()
(used to betoolbar_build()
). Why not move that code intotoolbar_pre_render()
?Comment #35
jessebeach CreditAttribution: jessebeach commentedThat code predates the current Toolbar update. It it makes sense now to change it, it's there for us to propose.
Comment #36
benjifisher@jessebeach:
Right, the issue I referenced in #34 was from 2009.
The attached patch is (almost) the minimal change to #27 that I think will make the Countcount module possible. It moves a few lines from
template_preprocess_toolbar()
totoolbar_view()
. I need some sleep, so I have not done any testing at all.If you accept this patch, then I think we still disagree on two points. Both of these affect the API that the module provides, but neither makes much difference (if any) to the functionality. (edit) Make that three.
toolbar_view()
should insert the'#theme_wrappers'
property where needed, so that modules implementinghook_toolbar()
are not responsible for it.'#tab'
and'#tray'
should both be nested inside the element returned byhook_toolbar()
.The first point makes it easier for one module to define a tab and another to hijack that tab to open its own tray. That is an edge case, and an even edgier case is an API module that defines a tray in case some other module wants to attach a tab to it.
I will try again to explain why I think the second point is the right way to go. If
'#tray'
is a property of the render element that defines the tab, then we allow any render element to be used for a tab unless the element type uses a'#tab'
property to mean something else. It may never come up, but I would feel silly documenting such an exception.For sorting, I do not have time to run an example and generate screen shots—and I will not have any time until Friday afternoon—but to see what I mean by "unpredictable sort order," try
Edit: Added sorting to the ordered list and discussion at the end.
Comment #37
jessebeach CreditAttribution: jessebeach commentedRight, right, right. I'm working on implementing a
'toolbar_tray_combo"
element throughhook_element_info
. I think that the end result of this work will be the ability to define a tray for any render element. So the second element in a list such as in your Countcount example. I'm 25% done (about) with this approach. I hope to have a patch posted today given my meeting load for the day doesn't impede too much.Comment #38
jessebeach CreditAttribution: jessebeach commentedMajor Changes
It took me a little longer to finesse the details, but I've got a patch I think we can work with. I ran these changes past Moshe at several points in the evolution of the patch before I finally landed on what's here.
What this patch does is remove the highly idiosyncratic processing of the
hook_toolbar
return values. We keep the structure of the renderable array passed back from the modules. But, because the trays need to be physically outside the.bar
element in the DOM so their position context isn't afixed
element, we move them in JavaScript instead.The trays are
display:none
by default, so we don't incur a repaint when they're moved.The root of the changes in this patch is the
toolbar_item
element defined intoolbar_element_info
.Modules can declare this type when registering components into the toolbar in
hook_toolbar
.A toolbar item without a tray
A toolbar item with a tray
A toolbar item with a tray and custom styling
The code also let's you implement your Countcount example without resorting to
hook_toolbar_alter
. I've provided two patches to your module in this issue that updates Countcount to use the new element defintion. One just updates the code to use the new element definition; the other removes thehook_toolbar_alter
code to get your example of a item list in the toolbar where the fourth item of the list opens a tray. This can be done right fromhook_toolbar
now.Summary
To get a toolbar tab with an associated tray, a developer will need to do 3 things.
#type
of a render array astoolbar_item
.l()
(unless a custom#theme
function is provided).tray
.Other small changes
toolbar_view
function as mentioned in #34.toolbar
intoolbar_element_info
that contains the defaults for a renderable toolbar element.toolbar.api.php
toolbar.module
so that similar functions are grouped. It had gotten quite jumbled.theme_toolbar
Patches
toolbar.module
to group functions.hook_toolbar_alter
has been removed. Apply this patch after the previous Countcount patch.Comment #40
benjifisherI will make a bunch of minor observations as I test. I will follow up with another comment after I have a chance to digest these changes (maybe tomorrow).
Minor point: I tried to apply the incremental patch on top of the one from #27. The patch failed.
I applied the two patches to the toolbar_example module. I will commit them to my sandbox: http://drupal.org/sandbox/benji/1862108.
I note that the "Click me!" link is a bare
<a>
element: since the module does not specify'#type' => 'toolbar_item'
(whether by design or by accident) it is not wrapped in the same<div class="tab">
as the other items, and it lacks the attributes they get.Originally, the "0, 1, 2, How many" links displayed horizontally, but now they are vertical. I do not really care: the example module is just an example, and in real life the module would be responsible for styling something this unusual.
The id attributes of the tabs and trays are now distinguished numerically, instead of by the keys returned by
hook_toolbar()
. I do not like this: how is a module supposed to provide custom styling for the elements it defines?The new version of the example module does not show how to add a form to the toolbar.
I tried restoring the deleted lines, and it did not work.(Edit: I tried a little harder, and it worked.)I added the alphabet example from #36 to the example module (and updated to use
'#type'
instead of'#theme_wrappers'
). It is about as bad as I expected. I think we need to sort the toolbar ourselves.I think that using a
'#type'
property instead of'#theme_wrappers'
is a big improvement.I do not like the idea of using javascript to separate tabs from trays. The only practical effect I can think of is very small: if I understand the caching strategy correctly, then we are asking the browser to do a little extra work when the page is reloaded.
If a tray is opened and the page reloads, I guess the server returns HTML with all trays in the default,
display="none"
, state; with the patch from #38, they are rearranged through JS; and then (with or without the patch) the tray is opened. If I have it right, then I understand your claim that this patch does not cause a repaint.I am not sure whether the toolbar_item type should use the key
'tray'
(a child element) or'#tray'
(a property or a variable for the theme function).Comment #41
benjifisher@jessebeach:
From #38:
An alternative is to use
render()
ordrupal_render()
(depending on where in the process we do it) on the tray elements. This will mark them as "printed", so we can then render the overall array and they will not be output a second time.A similar approach would be to use
hide()
andshow()
on the trays. This might make it simpler to render the result in the order we want.Back-end issues like this can be left for follow-up issues if we like. The part of this issue that is blocking others is agreeing on the structure for
toolbar_item
. I am still mulling it over.Comment #42
benjifisherSorting:
As I said in #36 and #40, I think we have to have a custom sort. My initial idea was to sort by title (after the primary sort by weight). That has some advantages and some disadvantages. I just thought of another disadvantage, which I think is decisive: if the site is translated, then the
'#title'
properties may change, leading to a different sort order. For predictable results, I think we should sort by the keys returned byhook_toolbar()
. This will also be simpler to implement than my original idea.Comment #43
benjifisherOK, on to the main design decision. What is the structure that should be returned by
hook_toolbar()
?First of all, I think the patch in #38 meets the standard of "I can live with it." It is flexible enough to handle all the use cases we have come up with.
However, I think the standard for this issue should be "Do we get it right?" rather than "Can I live with it?" We are designing an API to be used by other modules.
@jessebeach, I hope that one of us can win over the other.
I agree that defining the
toolbar_element
type is the right way to go. We agree that the tray should be a nested element. I am not sure whether it should be a child element or a property ('tray'
or'#tray'
).The question is whether the tab should also be nested, as in #17, or if it should be the main content of the element, as in #38 and others. I think it should be nested.
One way to decide is to make it work like other hooks. I think that
hook_block_view()
is a good analogy. Both'title'
and'content'
are children elements. It is not a perfect analogy: there is no'#type' => 'block'
that I know of.One thing you cannot do with the structure in #38 is assign a type to make your tab element render as a link, button, form, or other element. There can be only one type, and we have to set it to 'toolbar_item' (at least if we want to associate it to a tray).
OK, you get around the problem of not being able to specify a '
#type'
by specifying a'#theme'
. Or maybe a'#theme_wrappers'
. At some level, what specifying a type does for you is that it provides default values of these properties, viahook_element_info()
.This is a maintainability issue. If I write a contrib module and specify
'#type' => 'magic_pony'
, then the immediate benefit is that I let the Magic Pony module figure out what theme properties should be specified. Or process or preprocess properties. The long-term benefit is that the Magic Pony module can change its implementation, and I automatically get the the right theme wrappers applied. If I cannot specify'#type' => 'magic_pony'
, then I will have to update my module when this happens.Comment #44
jessebeach CreditAttribution: jessebeach commentedA module needs to pass in attributes to identify its renderables.
theme_item_list
doesn't add id information to the list about the module that called it for example. If a renderable with an associated tray is found deep within the array that a module returns tohook_toolbar
then we can't use the render system as-is to produce the output. We would need to parse the renderable and find instances of some special array key and add information to that array about its controlling module. This is not ideal and I struggled for hours to come to terms with this issue, ultimately deciding that toolbar can't be responsible for marking up content passed to it -- the calling module must do this.Why not just sort by weight and let the sort functions operate as they do? If fine-grained sorting is needed, then a module needs to pass in a
#weight
attribute.I think a module that implements
hook_toolbar
and passes a renderable with'#type' => 'magic_pony'
can still do so. If that type needs a tray, then it will need to provide the correct HTML classing anddata-*
attributes to associate a tray trigger with the tray element. The trays work off of HTML attributes -- nothing that a module couldn't provide on its own. Thetoolbar_item
has the advantage of making these connections automatically. But if a module really needs to go off the reservation, I don't think it's unreasonable that core doesn't support such an edge case.It's a bit late and I'm not completely following on the
render()
ordrupal_render()
issue you mention in #41, but it sounds like you're alerting to an issue with #children not being set bydrupal_render_children
?Using
hide()
andshow()
is an interesting approach. I'd thought of it but I just couldn't make it work, since a tray might found deep in the structure of a renderable, such as in the Countcount module. You need a predictable structure to use hide() and show() and we can't rely on or enforce that and also allow for arbitrary content to be passed back tohook_toolbar
. I'm happy to be proved wrong or shown a better way here!The reason I think that
#tab
should not be a called-out thing (with an array key) is because I adding content to the toolbar should be possible just by passing a renderable array tohook_toolbar
. If I can pass a renderable that also contains#tab
, I run the risk of defined two toolbar items with one renderable. Does that make sense?Comment #45
benjifisher@jessebeach:
This time I will start with the key point. We are not forced to render arbitrary content defined by
hook_toolbar()
, becausetheme('toolbar_item')
has the final say. See the description of the'#theme'
key on http://drupal.org/node/930760 (near the end of the main content, about half-way down the page counting the current comments). As I said,hook_block_view()
is analogous: a module can return any sort of renderable content, but only the'title'
and'content'
children get rendered.I will respond to the rest of your points in order. Several decisions depend on the key decision of how to structure the
toolbar_item
type.Yes, a module can define its own CSS classes and style them, but there may be cases where styling has to be applied to the wrapping elements.
Looking at the markup again, I think the current patch applies an id attribute to the tab element, overriding any that might be set by the module. I would like to see that id (such as
toolbar-tab-toolbar-tray--11
) applied to the wrapping div withclass="tab"
. OTOH, the tray has the id attribute attached to the wrapping div.If the tab and tray are both top-level children of the
toolbar_item
, then it should be easy to give them (that is, the wrapping div's) id attributes based on the key.Individual modules should not be responsible for fine-grained sorting. That should be left to the site builder. One alternative is to offer an admin page where the site builder can rearrange the tabs. (This is definitely a follow-up issue!) That would be a good way for the Toolbar module to implement
hook_toolbar_alter()
.I feel strongly that we should encourage most modules to stick with the default (no
'#weight'
property, which is treated the same as'#weight' => 0
) AND that the sort order should be predictable.I agree that the Countcount example is an edge case, and that a module following this pattern should be expected to do extra work. I just think that the approach I suggested, using
hook_toolbar_alter()
, is simpler than the approach you suggest. The Toolbar module should be the only one that needs to know the details of the data-* attributes.The same point I made about maintainability at the end of #43 applies here. If Toolbar changes its implementation, then I have to update my module. We avoid this problem by using
'#type' => 'toolbar_item'
in the top-level element and'#type' => 'magic_pony'
in the'tab'
child element.Secondary issue: as long as the sort is unpredictable, it is impossible for the module to set the data-* attributes itself.
No, I am not aware of a current problem with
'#children'
not being set. I was just suggesting alternative ways (render()
ordrupal_render()
orhide()
andshow()
) to separate the tray markup from the tab markup. I agree that this is impractical using the current structure, but if each toolbar_item element has'tab'
and'tray'
children, then it should be easy.Comment #45.0
benjifisheradded images to the use cases
Comment #46
jessebeach CreditAttribution: jessebeach commentedCorrect, I tried to provide for this. As a renderable passes through the various stages of pre rendering, user-provided values are preferred to theme-provided values. So if you set an id or a class on a renderable, that id will be used instead of the one produced by the default functions.
I also provided a key
#wrapper_attributes
that a developer can use to pass attributes to the wrapping functions.Both these issues of overrideability came up as I was testing the use cases we defined in the summary.
On the sorting issue, I'm happy to see something better than what's there, but personally I'm fine with the standard sorting behavior since any secondary sorting, as far as I can predict, will be based on assumptions that may not be true in all cases i.e. that items of equal weight should be sorted by alpha on title.
Sorting will have nothing to do with the data-* attributes. As long as the
data-toolbar-tray
attribute is the same on the tray and its trigger, the JavaScript will associate them, regardless of their placement in the DOM.Comment #47
benjifisherIn that case, I retract the objection that modules cannot set the data-* attributes. When I wrote that, I was just looking at the markup, not the code, and what I saw there is that the data-* attributes involved the numerical sort order.
Comment #48
benjifisherFollowing up on one of my points from #45, I added #1869638: Make the menu shown in the administration menu tray configurable.
As I contemplate building a configuration page for the toolbar, it occurs to me that
hook_toolbar()
should define'#title'
(or'#name'
) and'#description'
properties for each item it returns. These may often be the same as the corresponding properties for the tab, but I do not think we can rely on that. This is one more reason that IMO the tab should be a child element of the toolbar_item.Comment #49
jessebeach CreditAttribution: jessebeach commentedbenjifisher, if we place the "tab" content under a
#tab
key, what would we do in this case?Do we render both the search box and the home link "tab?" So each module can place two items in the toolbar with each keyed item returned to
hook_toolbar
?This is the case I'm stuck on for the reason I don't think we can bury the content that should be rendered to the toolbar in a keyed array inside the array that we pass back to
hook_toolbar
. The contents of the array passed back tohook_toolbar
should be the thing rendered to the toolbar. The fact that it has a tray is completely ancillary.That all being said, your use case in #48 for configuration makes sense. If we make
hook_toolbar
more likehook_menu
then it makes sense that the array passed back tohook_toolbar
has a more rigid array key definition. I can back that. I'm reluctant to over-optimize the code in this issue, though. I'd rather leave configuration to its own issue and adjust the signature of the return array in followup issue when the focus is specifically configuration.The patch in this issue makes
hook_toolbar
more of a pass-through for renderables and tries not to make any presumptions about the structure of the renderable except for thetray
key. We're still in feature completion, we can continue to refine the code as use cases mount. What do you think?Comment #50
benjifisher@jessebeach:
The example that worries you is inside out. As I see it,
$items['global_search']
should have'#type' => 'toolbar_item'
. If you want a search box to display in the toolbar, then$items['global_search']['#tab']['#type'] = 'search';
; If you want a link, then$items['global_search']['#tab']['#type'] = 'link';
.As I said in #46 (first point) once we require
hook_toolbar()
to return items of typetoolbar_item
, we get to decide what gets rendered because we get to choose the theme function. The attached patch shows how to do this: I have removedtheme_toolbar_tab()
and addedtheme_toolbar_item()
. This function does nothing but render the'#tab'
property, leaving the'#tray'
for later.I agree that we should leave configuration for later. That is why I added #1869638: Make the menu shown in the administration menu tray configurable as a separate issue. I am not sure what you mean by "adjust[ing] the signature of the return array", but I think I disagree. As I see it, the point of this issue is to finalize the API. I think the configuration page is just another use case, and we should design the API with it in mind.
I think we agree on the goal. It seems to me that your
theme_toolbar_tab()
does make a presumption that the tab is a link element.I am still not sure whether
'#tab'
and'#tray'
should be properties or children. If we are going to define defaults for either one inhook_element_info()
, then I think they should be properties. I looked at all the core implementations ofhook_element_info()
, and onlyfield_ui_element_info()
defines a default value for a child. That looks to me like a mistake rather than a precedent.The attached patch does not do much more than necessary to make the tab a nested property inside the toolbar item. There are a lot of other things I would like to change, but first let's agree on the API. There are certainly comments and tests to be updated, plus the .api.php file. Let me mention just one more thing for now: the line
inside
toolbar_pre_render()
. It seems to me that rendering inside a pre-render function short-circuits the usual rendering process.Comment #51
benjifisherI am setting this to NR to see what the testbot thinks. I still think the patch needs work.
I have also attached an incremental patch (interdiff).
The attached patch does two things. First, it updates the test module and the API documentation to be consistent with the patch in #50 (see below). Second, it uses PHP instead of javascript to separate the tabs from the trays, undoing oart of the change made in #38.
@jessebeach, I tested briefly with the ".tray" div's inside the "#toolbar-bar" div, and it seemed to work properly. At what point do you see a problem?
I guess I have not been clear about the structure I am suggesting, with a toolbar-item render element having '#tab' and '#tray' properties. To be explicit, here is a snippet from my version of user.module:
I hope this clarifies what I meant when I called the snippet in #49 "inside out."
Looking at this again, I wonder whether the '#heading' property should be part of the '#tray' element or perhaps the parent element (type toolbar_item) should have a property '#tray_heading'.
Comment #53
jessebeach CreditAttribution: jessebeach commented#51: toolbar-refactor-1847198-51.patch queued for re-testing.
Comment #54
jessebeach CreditAttribution: jessebeach commentedTests are running fine locally. I requeued #51.
I'm ok with the patch in #51. Let's leave this as Needs Review for a week to let others chime in. @benjifisher, I think we've reached a point of consensus that allows us to move forward. Getting this code in and working with it will let us know if further tweaks are needed. Sound good?
Comment #56
Bojhan CreditAttribution: Bojhan commentedI see a whole bunch of CSS changes, does this have any UI changes?
Comment #57
benjifisher#38: toolbar-refactor-1847198-38.patch queued for re-testing.
I am getting the same failures locally as the testbot. Technically, not failures but exceptions. It is the breadcrumb test: I was not expecting a problem there.
I am now racing with the testbot: can I figure out what is going wrong before it finishes this test?
Comment #58
benjifisherThe problem is the Edit module, which added hook_toolbar() in #1824500: In-place editing for Fields. The attached patch is the same as #51 except that I have updated edit_toolbar().
Comment #59
benjifisher@jessebeach:
Can you answer @Bojhan's question in #56? (and mine in #51, just before the
<hr>
)There are a few changes I still want to make, though not necessarily before closing this issue:
OK, #1 does not affect the API, so it can certainly wait for a follow-up issue. #2 and #3 both affect the API, so I would prefer to address them here. I am willing to go along with putting them in follow-up issues in the interests of helping the Edit module, especially if it seems likely that no one will be using hook_toolbar_alter() nor targeting the id tags with their CSS just yet.
Comment #60
benjifisherI am working on the issue summary.
Comment #61
jessebeach CreditAttribution: jessebeach commentedRE #56, The CSS changes replace
li
with.tab
. There are no visible effects of these changes.To position the trays reliably and consistently, we need the positioning context of the tray elements to be
absolute
. The#toolbar
element that contains both the bar and the trays is set toabsolute
and this does not vary. The bar on the other hand, switches positioning fromabsolute
tofixed
depending on the media query. When the position of the bar isfixed
, then so are the trays in effect; they won't scroll with the content and we need scrollbars inside the trays to deal with an overflow of vertical content.I don't want to over-optimize the toolbar for themers without use cases for how and why they would need easy access to manipulate aspects of the Toolbar's structure and styling. Module developers might add a top-level item, but even this should be done only in cases when a top-level item is really warranted. Otherwise administrative links should be located in the administrative menu. Themers shouldn't be restyling the Drupal administrative toolbar under normal circumstances.
Comment #62
sunThanks for working on this, and also for the elaborate discussion.
There are various goals and tasks involved in this issue, which haven't been summarized clearly yet, so let me give it a shot:
My first and foremost recommendation is to take a step back and have a look at the situation we're facing architecturally:
This immediately leads to the conclusion that the trays should be entirely detached from the tabs. In other words, a tab only specifies which block should be rendered for it when it is toggled (if any). But the tabs/tray's block is not necessarily rendered in advance, which inherently means that its render structure should also not necessarily be built ahead of time.
This clear separation has a range of positive consequences: Tray blocks are just blocks; any block is suitable. Once Blocks/Layouts and WSCCI are fully in place, complex/large blocks (such as a fully rendered admin menu link tree) can automatically leverage the entire architectural plumbing and infrastructure that exists for building, rendering, and most importantly, caching of blocks. And of course, developers don't have to learn a custom API for producing toolbar tray block contents.
In turn, it helps to shift the perspective on the toolbar from "A list of links" to "A layout consisting of content blocks as well as various JS-driven triggers that load and expose content blocks in a content region of the layout."
As a result, the technical implementation should account for:
The render array structure for each "toolbar item" should therefore not contain the tray/block content. @benjifisher's proposal of separate 'tab' and 'tray' keys comes closest to that:
But we actually need to go one step further and detach the tray block content from the tab item entirely:
Now, if someone were to Ajax-ify or ESI-fy the entire toolbar leveraging D8's Blocks/Layouts/Services New World Order™, then that would translate into:
In other words, the actual tray block contents can either be pre-generated ahead of time via internal sub-requests, but they can also be omitted altogether and get dynamically requested purely from the frontend only instead.
That would technically lead us to the following hook definition/structure:
There's only one remaining tidbit with that: The above list of goals states that a rendered tab may not necessarily be a toolbar tab (link) and/or that the actual tab/triggering-element is only one sub-element of the rendered elements.
Typically, one would resolve that last detail by doing two things:
.toolbar-tray-trigger
HTML class.In turn, 1) multiple elements within a tab can trigger the tray, 2) arbitrary elements (not only links) can trigger the tray, 3) a tab may not trigger a tray at all, 4) the tray can be triggered/toggled via external JS/Ajax.
In any case, I cannot make much sense of the toolbar_item, toolbar_tab, toolbar_tray, and the other proposed theme/wrapper functions. I believe they are wrongly and too focused on the interpretation of toolbar as "A list of links."
In general, we should use non-special standard markup for the toolbar tabs and trays. We should treat the toolbar as a layout:
Each of the DIVs essentially wraps a toolbar item ("tab"). The toolbar tab item's content can be anything. It can be a simple link with or without a .toolbar-tray-trigger class. It can also be a search form. Which may or may not load search results through an Ajax POST request and manually trigger the tray. It can also be plain text - which may or may not be wrapped in a SPAN or DIV having a .toolbar-tray-trigger class.
The necessary CSS plumbing should happen at the layout's region level, and for the upper two regions, the wrapping DIVs should be the basis for applying the special toolbar layout styles. Speaking of, I'm not sure whether I see a point in allowing items to opt out of the toolbar tab styling in those regions — the one and only styling detail I could foresee is that a tab/item might want to "visually merge" itself to the left or right; i.e., omitting a potential tab border to a preceding or following tab item.
Likewise, the toolbar tray block contents should not require special markup to work within a toolbar tray. E.g., the admin menu simply is a rendered menu link tree. There should be sane default styling for various HTML elements that may appear within a tray; potentially/ideally triggered by a HTML class only (so there is a way to opt-out of the default styling). And we can attach additional CSS and potentially even JS to a particular tray block content to make it work better within this particular toolbar tray context (e.g., to hide deeper menu levels in a menu link tree and inject toggles to expose them).
In summary:
Apparently, I did not provide a concrete proposal for the last point yet. There are multiple possible ways for doing so. One central aspect is the question of whether it should be possible for one tab/item to trigger the tray of another item; i.e., re-use of trays, which would turn the tab:tray mapping from 1:1 into n:1. However, while I can see technical arguments for that, off-hand, I can't see a real-world use-case for it, so I'd like to ignore that for now.
The most simple way would be to mix a custom #property into the top level of the render array for a tab/item render array definition; e.g., like this:
Let's bear in mind that one module (hook implementation) should be able to return more than one toolbar item; therefore,
$items['account']
is the "top level", not$items
.Another way would be to turn only the second level into the actual render array for the toolbar item, and have the first/top level be a non-render array declaration; e.g., like this:
And once we arrived at that conclusion, it's guaranteed that there will inherently be someone who will ask the fundamental question:
A perfectly reasonable question. In turn, that would translate into:
In turn, that would have the positive consequence that each toolbar tab/item is a (~regular) block itself. Which inherently means that you can potentially add e.g. the existing Search Form block as a tab/item to the toolbar, without having to code it up first (which may not be fully true for every block, since custom CSS/JS and potentially Ajax may need to be attached to make it work in the toolbar, but regardless of that, it significantly increases possibilities).
Comment #63
jessebeach CreditAttribution: jessebeach commentedMuch food for thought. Thank you sun for the thorough feedback. benjifisher and I will walk through it and incorporate it.
Comment #63.0
jessebeach CreditAttribution: jessebeach commentedupdated the proposed resolution
Comment #64
benjifisherI am done rewriting the issue summary. I preserved and updated jessebeach's addition to the Remaining tasks. I hope I did not lose any other updates.
I agree that the title needs to be updated, but
toolbar_view()
is obsolete. Here is another new title.jessebeach: Thanks for the clarification about separating tabs from trays. I see why my limited tests did not run into any problems.
I added some follow-up issues, including one to continue the discussion from #45–47. If I am the only one who thinks it needs changing, then there will be no harm beyond a couple of lonely issues.
@sun:
Thanks for the new ideas in #62. From the start, my goal on this issue has been to address your request in #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop that the toolbar be made flexible enough that you can build on it for the Admin menu module.
There are some important points where we agree.
That has been my idea since my first patch on this issue, and it is incorporated in the patch in #58. I am sorry I did not finish reworking the issue summary earlier; I think I might have saved you some trouble. I think this one point handles most of the list of "goals and tasks" mentioned at the top of #62.
I think this is the point of the theme wrappers. If you mean that the Toolbar module pre-render functions should be responsible for adding the theme wrappers, and the module implementing
hook_toolbar()
should not have to know about them, then I agree. I need to spend some more time looking at these.I think we all agree on the three points listed under this in #62, except that so far "arbitrary content" is interpreted as "any render array" rather than "any block." I am afraid I do not follow the next part: "This immediately leads to the conclusion that the trays should be entirely detached from the tabs." What are the use cases?
There are some points where I have little to contribute but questions. Patches are always welcome ...
Currently, the tray can be any render array. I do not know enough about "Blocks/Layouts and WSCCI" nor plugins to be of any help in replacing the render arrays with anything more general. The best I can hope for is that the structure we have defined so far is good enough that it can be modified later: the toolbar_item element will still have a
'#tray'
property, but in the future it may be something other than a render array.Question: can we use render arrays as wrappers for blocks or plugins?
jessebeach and I have been debating how to allow a sub-element of a tab be the trigger. We have tentatively agreed to postpone this decision, and I have added a new issue for it: #1877468: Finalize usage of hook_toolbar_alter(). (My description in that issue may not make the connection clear.) I am willing to deal with the decision in this issue, and it would help to have input from others, since so far jessebeach and I have not been able to agree.
I know enough about layout in CSS to realize that it can be hard. I defer to jessebeach and others, and I do not think it is part of the scope of this issue.
The main decision that has to be made in this issue is the API provided by
hook_toolbar()
. IIUC, the main API change proposed in #62 is to separate the tab from the tray. The current structure is(As I said above, I cannot comment on the merits of making the tray something other than a render array.) One significant advantage of this approach, keeping the tab and tray as properties of a single render element, is that we can guarantee "a reliable HTML ID" (borrowing a phrase from #62).
I am not sure what alternative you are proposing for
hook_toolbar()
. I suppose one option is to have two hooks, one to define tabs and one to define trays. I think I prefer something like this:(With this structure, it might make more sense to replace the
'#tab'
and'#tray'
properties with children ... but then we would have to figure out what to do if there are multiple children.) Whether or not this is close to what you had in mind, my main question is how we guarantee that the tab and tray use a consistent "reliable HTML ID".I am not sure how this would work, and I am not sure that this would make it easier for a module developer. Perhaps this is the same as the previous question.
Comment #65
jessebeach CreditAttribution: jessebeach commentedThis is just a quick message about process now that we have a functional-but-non-ideal solution in patch in this issue and a better-but-yet-uncoded improvement proposed by sun.
Sun, you gave this example in #62 of what might be considered a half-way point for the ultimate improvement of the Toolbar item registry.
-- #62
The patch in #58 brings us this far. Here's an example from the toolbar.api.php file that is similar to your proposal.
$items['user_messages'] = array(
// Include the toolbar_tab_wrapper to style the link like a toolbar tab.
// Exclude the theme wrapper if custom styling is desired.
'#type' => 'toolbar_item',
'#tab' => array(
'#type' => 'link',
'#theme' => 'user_message_toolbar_tab',
'#theme_wrappers' => array(),
'#title' => t('Messages'),
'#href' => '/user/messages',
'#options' => array(
'attributes' => array(
'title' => t('Messages'),
),
),
),
'#tray' => array(
'#heading' => t('User messages'),
'messages' => array(/* renderable content */),
),
'#weight' => 125,
);
return $items;
Now I realize this isn't exactly the same as what you've proposed. But it does move us one step closer to a better design and more importantly, it allows us to remove custom code from Edit module that addresses the shortcomings of the current Toolbar architecture.
So what I'd love to do is do a few small cleanups on the patch in this queue and move the plugin/blockification to a followon so we don't have to wait several more weeks to clean up Edit. The new issue will also allow us to focus the discussion on the improvements that Sun suggests rather than introducing a pivot to this issue already 60+ comments long.
What do you both think? We can do a little more clean up here, get this incremental improvement in and continue to work on it further in a followon. If that's acceptable, I'll put the followon task together with details from this issue that describe the approach.
Comment #66
benjifisherjessebeach:
I would like to do additional work, but I agree that there is a strong case for committing something without getting close to the February 1 deadline. The work I want to do could be attached to follow-up issues.
The one thing I think should be settled here is the API. Unless someone else comes up with a patch soon, I do not think we can move away from render elements. (I do not know enough to move in that direction.) But there is still Sun's suggestion in #62 that "the trays should be entirely detached from the tabs." I responded to this suggestion in the last section of #64. I am afraid that I do not follow Sun's logic, and his
$items['account']
snippets are missing enough that I am not sure exactly what he is suggesting.You suggested in #54 a 1-week comment period. I am willing to go along with that, meaning that we recommend commiting the current API unless there are further objections, clarifications, or patches by January 7. For my part, I will (for the sake of having a stable patch) not offer anything new except possibly improving the documentation.
Comment #67
jessebeach CreditAttribution: jessebeach commentedThis is a reroll of the patch in #58 to incorporate changes to Edit module.
We acknowledge that this patch is not everything we want right now. It is a measured step forward that addresses several shortcomings of the current Toolbar hook architecture. More work will need to be done, specifically moving on sun's suggestions in #62 and benjifisher's responses in #64. I've created a followup issue here that pulls the info from those comments so we can focus on them exclusively.
#1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it
Moving on the patch in this issue will allow us to address the following issues now:
#1860436: Remove toolbar-specific code from the Edit module
#1678002: Edit should provide a usable entity-level toolbar for saving fields
The following describes the changes this patch introduces:
These improvements address several of sun's suggestions in #62:
We are punting in this issue on whether toolbar items should be plugins and whether tray content should be loosely associated with a triggering tab. That will be hashed out in the followup.
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedThis just moves toolbar.module functions around to match their location in HEAD to cut down on the patch noise. If you intended to reorganize order of functions, please do so in a separate issue.
I'm also not quite clear on why we're using properties instead of children to store render arrays, but my first impression is that that's wrong. I'm still getting up to speed on the patch though, so maybe there's something subtle I'm not seeing yet.
Comment #69
effulgentsia CreditAttribution: effulgentsia commentedIt's possible that this is totally broken, but this changes '#tab' and '#tray' properties into 'tab' and 'tray' children. If anything is broken by this, I'm pretty sure we can fix it, so please report what you find.
Comment #70
jessebeach CreditAttribution: jessebeach commentedbenjifisher and I have been going back and forth on this detail in our little echo chamber. Having some outside input helps precipitate a direction.
This patch changes instances of
#tab/#tray
totab/tray
.Comment #71
effulgentsia CreditAttribution: effulgentsia commentedI think some of the toolbar.module pre_render/preprocess/theme functions in this patch can be cleaned up some more, but it's late, and I'm too tired right now to communicate detailed thoughts on that. In the spirit of iterative development though, I think that what's here is a very nice improvement to what's in HEAD, especially with respect to the API of hook_toolbar(), and therefore, should go in, and improving the internal rendering flow further can be worked on in a follow up. Similarly, #1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it may change some of what's here, but I think what's here is still a worthwhile interim step. Therefore, RTBC.
This just moves #access out of toolbar_element_info(), because the result of that hook is cached globally, not per-user.
Comment #72
webchickTagging.
Comment #73
webchickThis looks a lot more straight-forward to me. Thanks a lot for all the hard work here.
Given that there are numerous features bound by feature freeze bound on this patch making it possible to put anything but links in the toolbar (escalating to major in response), I'm inclined to commit this. I'm skeptical about both the performance and DX implications of a plugin-based approach, but we can cover that in #1894964: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it.
Committed and pushed to 8.x. Thanks!
I don't think this really needs a change notice, because it's a new feature anyway. But it might need an upgrade to an existing change notice.
Comment #74
effulgentsia CreditAttribution: effulgentsia commentedPer #71, I'd like to help clean up some of the toolbar.module code, but let's start with just a docs-only cleanup.
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedAdjusting priority to reflect remaining work.
Comment #76
jessebeach CreditAttribution: jessebeach commentedI ported the patch in #74 to #1894964-1: Make the Toolbar PHP and JavaScript API more flexible so that it enables contrib to leverage it in order to kick off the follow-up issue.
Comment #76.0
jessebeach CreditAttribution: jessebeach commentedUpdate proposed resolution, API changes, and other parts now that we are nearing consensus.