Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This patch provides Vertical Tabs integration for the block visibility settings on the block configuration and addition form.
Comment | File | Size | Author |
---|---|---|---|
#67 | block-vertical-tabs-followup-556390-67.patch | 2.05 KB | David_Rothstein |
#65 | page-visibility-before.png | 38.14 KB | David_Rothstein |
#65 | page-visibility-after.png | 37.97 KB | David_Rothstein |
#65 | block-vertical-tabs-followup-556390-65.patch | 2.05 KB | David_Rothstein |
#62 | VT-blocksettings.patch | 14.21 KB | yoroy |
Comments
Comment #1
Dries CreditAttribution: Dries commentedIf we do this, I think we should rename 'Page specific visibility' to 'Page visibility', etc. The titles shouldn't wrap, IMO.
Comment #2
davyvdb CreditAttribution: davyvdb commentedHere's a patch with the shorter titles.
Comment #3
yoroy CreditAttribution: yoroy commentedCritical status is only for stuff that's broken, not the case here.
It's probably ok to apply the vertical tabs here, but I'm not really sure. Do we have use cases where you want to use combinations of page & role specific settings for example? If so, then VT is not very handy, becuase it would be harder to compare two or more settings. If the 80% use case for a block is that only one of these settings is used to control visibility, then it's ok.
Comment #4
Dries CreditAttribution: Dries commentedMy experience is that the 80% case is to only use one, but I have certainly used more combinations.
Comment #5
davyvdb CreditAttribution: davyvdb commentedI used Vertical Tabs here because all these fieldsets have to do with the same subject "visibility". Vertical Tabs makes this look like a group where you can see what your options are. Vertical Tabs is not used throughout core as a mutual exclusive options pattern.
Comment #6
yoroy CreditAttribution: yoroy commentedYes it is. It's one of the most important aspects to consider when thinking about applying this pattern.
Like how it's used on the node edit form. There's no need to see a revision option if you just want to change the author.
Same for the user e-mails in user settings, though you might want to re-use some text, you don't need to see one to be comfortable in editing another. Like Dries says, most of the times its ok here, but I'm not sure if we really win anything here.
Comment #7
davyvdb CreditAttribution: davyvdb commentedOK I see. But to be honest. The fieldsets (4) are a PITA right now if you want to review all your settings quickly. If you open a fieldset, you have to scroll down and up to go the other ones. It's easy to miss one too because they move. With Vertical Tabs, your clickable titles stay in the same place. So you can review all your settings very fast.
Comment #8
Dries CreditAttribution: Dries commented@yoroy, good point on the pattern. That might be worth documenting in the vertical tabs API to avoid 'abuse'? Also, so how do you suggest we proceed on this one?
Comment #9
Bojhan CreditAttribution: Bojhan commentedI dont think this is a win, indeed Vertical tabs is an faster interaction but in a lot of cases you don't need to expand all of the fieldsets. You know what you are looking for in this case, while Vertical tabs is usually used more as a way to quickly go trough additional functionality/meta data - in this case its the most critical functionality of this whole page.
I say we don't continue down this path.
Comment #10
seutje CreditAttribution: seutje commentedI like this, since I mostly only use the page specific settings and I don't like scrolling in general
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commented@Bojhan - please address the benefit described in #7 where you can see at a glance the summary of each fieldset and then dive into just the ones that need changing. That's the main benefit of this pattern IMO - the summary. I think it is useful here.
Update: isn't collapsed fieldsets the same as vert tabs but lacking the summary? In what way is a bunch of collapsed fieldsets superior?
Comment #12
Bojhan CreditAttribution: Bojhan commentedOke, so right now I am somewhat guarding the vertical tabs because from where it originated it is designed for the node/add page - we can indeed apply it on other places, but with great caution. There are a lot of reasons but let me explain the most prominent ones.
Useage
We should only use Vertical tabs for meta data or otherwise not necessarily to configure information. Although you can skip these items, they are apart from the title more often than not the only forms on that page agreed? Which turns these vertical tabs into the main source of navigation for one of the most prominent interactions. The intend of Vertical tabs, is not to be the main interaction in a form - because users should be able to skip a vertical tab. If we apply it differently, some you can skip - some you can, but not really skip, and some you can't - people will be required to tab trough all the VT's.
I don't see it as clear that, vertical tabs is just the same as fieldset but with a summary. Yes, it does certainly look that way, but the way they are applied is far differently. We could argue that fieldsets are not good by nature on this page, because they don't expose any summaries - but then again, this page is about configuring something you know what to look for. Vertical tabs tend to be used as something to to fiddle trough.
Design
So onto the current design of this patch, we have vertical tabs that are more then one line. For scanability, we need to assure that each vertical tab is always just one line. This also applies to the descriptions, and I am unsure how we could improve that without losing information value.
Sorry, if it seems like this is an obvious improvement - and I am disagreeing. But I am here to stand guard of behavior that we saw in user testing that we wish to preserve. We spend a great deal of time, making sure that this pattren is used consistently and that should for Drupal 7 mean that people use them the same way as we observed in user testing.
Comment #13
davyvdb CreditAttribution: davyvdb commentedHas this pattern (grouped settings) been tested in user tests?
Comment #14
Bojhan CreditAttribution: Bojhan commentedYes, not in this particular place.
Comment #15
yoroy CreditAttribution: yoroy commentedYes, vertical tabs were tested in Baltimore earlier this year: http://www.drupalusability.org/search/node/vertical
They were tested as part of the node add form and were a succes there because for the most part people *ignored* them. 'Invisible design'; don't bother people with less important stuff, basically.
Moshe: one advantage of expandable fieldsets is that you can expand multiple simultanously, should you want to compare dependable stuff. This is impossible with VT. the summary can only partly do that and probably not enough in this case of blocks settings.
Comment #16
catchWhile I might use more than one visibility setting (although not very often), I never need to compare the values here side by side, and the fieldsets are usually long enough that do so means scrolling up and down anyway - it's big textareas, long lists of checkboxes for roles etc. - so the proximity seems like a good thing here.
Comment #17
RobLoachThis addition makes sense and really cleans up the page a bit. As catch said, you never have to compare these settings side by side, and even if you do, the summary on the side provides hints towards its contents.
The attached patch adds hints for both "Role specific visibility settings" and "Content type specific visibility settings" when there isn't a selected option. You can see it in action under the Seven theme in the provided screenshot. It also moves to #attached_js for adding block.js.
Regarding the usability, the majority of the participants seemed to like the vertical tabs, but got confused when it came to the order in which they appeared. So, should we re-evaluate the order in which they appear? Thanks.
Comment #18
TD540 CreditAttribution: TD540 commentedI definitely approve of this. And it would simply add more consistency across the Drupal admin.
But, how about grouping these into one VT group titled "Visibility Settings" (simply by adding a title above the page-/role-/contenttype-/user tabs)
and then, if other modules want to add certain settings tabs, it would be neat if they could also be grouped, either by function, or by module, or whatever that made sense.
just a thought.
Comment #19
davyvdb CreditAttribution: davyvdb commentedTitles on Vertical Tabs are not supported yet (afaik). Extending can be easily provided using hook_form_alter. Thx for the thoughts!
Comment #20
yched CreditAttribution: yched commentedIf we go this way (which I think is neat), I'd say we can remove the 'Block Specific Settings' fieldset.
Comment #21
RobLoachDavy Van Den Bremt at #19
Do you know if there's an issue open for titles on the vertical tabs?
Comment #22
davyvdb CreditAttribution: davyvdb commentedFieldset is not collapsible now. Fieldset title is now Identification since nothing else is in there and to match this with content types edit : admin/structure/node-type/article
Comment #23
stBorchert@Davy: is there a reason why you didn''t move "Region settings" into the vertical tabs?
No need to shorten this. Use "block_visibility_settings".
same as above (and in all other fieldsets)
This review is powered by Dreditor.
Comment #24
davyvdb CreditAttribution: davyvdb commentedI wanted to group visibility settings in the tabs.
Comment #25
Bojhan CreditAttribution: Bojhan commentedI love that you guys are working on this, but I am almost tempted to mark this won't fix. As explained in all the arguments above? Ignoring them and continuing on the implementation won't make them less valid.
Comment #26
davyvdb CreditAttribution: davyvdb commentedBojhan. Sorry that we're a bit stubborn on this. But the more I read your reasons I'm getting more and more convinced that admin/structure/node-type/article and admin/config/people/accounts > e-mails shouldn't have vertical tabs either.
Mostyl "they were tested as part of the node add form and were a succes there because for the most part people *ignored* them. 'Invisible design'; don't bother people with less important stuff, basically."
Comment #27
mgiffordI think it looks better with the visual tabs.
The patch applied cleanly.
I am worried though about creating more vertical tabs before dealing with the accessibility challenges they provide http://drupal.org/node/467296
Comment #28
Bojhan CreditAttribution: Bojhan commented@Davy Van Den Bremt At admin/config/people/accounts, you can actually ignore them. I know its a very fine line, I believe that is actually the only good application of this pattren in core beside node/add
Comment #29
davyvdb CreditAttribution: davyvdb commentedFair enough. I have no more arguments. Who will be having the final call on this? In case of a negative one, I think this should then be made really transparent to contribute module developers. Because it's very tempting to use this pattern.
Comment #30
janusman CreditAttribution: janusman commentedI kind of agree that creating content (node/add) is a place that is very very frequently used, hence it needs to be streamlined (meaning: compact or hide stuff via VTs).
It will probably be frustrating for (beginner?) admins to "not see" important things like block visibility settings... but I agree, it's a fine line. "Reduce the amount of scrolling" is not really the problem to fix, it's making all the information grokkable.
And, I'm also worried that, just because the code is there, we try to apply them everywhere, so +1 for documenting these sort of controls (heck, I still have discussions about using checkboxes vs. radios vs. selects with seasoned Drupal ppl) =)
IMO I like VT usage here, in that it's grouping different types of visibility settings; once you understand one tab controls visibility, (I assume) you will think the others do something "similar" (which they do). So, if it matters, this has my vote...
Comment #31
davyvdb CreditAttribution: davyvdb commentedI've been thinking about how I use the block configure screen again. I must agree I fall back to php visibility for user role visibility too, because I always overlook the role specific fieldset. Using VT to group all similar settings make you indeed look further for other options.
I need this pattern a lot actually. And there is no construct to do this except for VT in Drupal for now. Maybe we should use VT to group similar settings too for now and think of a better construct for this in Drupal 8. I don't think there are much other candidates for this pattern (group simmilar settings) in core now.
Comment #33
yoroy CreditAttribution: yoroy commentedComment #34
Cliff CreditAttribution: Cliff commentedLike mgifford, I wish that some of the energy devoted here could be transferred to the issue of accessibility of vertical tabs.
Here's one reason these two conversations should be linked: Bojhan said, "Users should be able to skip a vertical tab." If that's the case, then we clearly need the text in these tabs to be headings — because that's the only way that a person who must rely on a screen reader can get a list of just those items and skip to any one or more that they need to work on. (By the way, at #19, when Davy was talking about titles, did he actually mean headings?)
By the way, if anyone involved in this issue knows JQuery, your help is sorely needed at http://drupal.org/node/467296.
FWIW, I'm not sure I grok the party line on when vertical tabs should and shouldn't be used, but it does seem to me that a couple of users here have made a pretty good case for them in this instance.
Comment #35
sunThe region settings are the only settings here that are required. Everything else is totally optional, and it's rather unusual that one wants to combine multiple visibility settings.
Hence, vertical tabs are a valid instrument. (According to Bojhan's own explanation.)
However, feedback on latest screenshot in #22: (http://drupal.org/files/issues/Picture%201_0_135.png)
1) There should be no fieldset for the primary block "identification" itself. Just drop it.
2) The region settings fieldset should be uncollapsed and _not_ collapsible.
3) The vertical tab pane titles need to be shortened, as already outlined in the beginning of this issue: Page visibility, Role visibility, Content type visibility, User visibility. In addition to that, "Role" should come after "Content type".
Comment #36
Dave ReidI completely support this. Revised patch that moves the 'Regions' fieldset into the 'Visibility settings' vertical tabs because it affects visibility as well. Moves the normal block settings fields to the base form element instead of it's own fieldset as per sun. Also includes summaries for all fieldsets and fixes default values for the radio buttons.
However, the tabledrag code was failing when included on the individual block settings page, so I added a check if Drupal.tableDrag exists. Now, on the block listing pages, the vertical tabs code conflicts. So there's a problem here.
Also includes before/after screenshots.
Comment #38
sunAwesome. Let's do the following:
Comment #39
Dave ReidWe can't really do 'Not restricted' for the page visibility since it's 'All pages except for those specified'. That would however be an option with #514256: "Show on all pages" - block specific settings.
Made the other changes and removed the silly
t('!theme', array('!theme' => $theme->info['name']))
since we don't translate theme names.Still need to solve the JS conflict on the block listing page.
Comment #40
sunThere _must_ be an additional key, so please revert the removal of the 'block_settings' element key. We can shorten to 'settings' though.
Somehow, this is missing an array().
Ideally, we put all of these into a dedicated form element key, i.e.:
$form['visibility']['page']
That said, the internal key should be 'path' here, IMHO.
Likewise, the internal key should be 'node', IMHO.
1) $.trim()
2) Shouldn't the regions already be check_plain'ed?
Why are these ^anchored?
This review is powered by Dreditor.
Comment #41
Dave ReidRevised patch that fixes all the previous concerns. Still has JS conflict.
Comment #42
sunThe added condition for Drupal.behaviors.blockDrag is correct. So no worries. Ideally, those two behaviors would not live in the same file, but well.
I did some wording, condition, and coding style tweaks, and I really think this is AWESOME and ready to fly.
Comment #43
David_Rothstein CreditAttribution: David_Rothstein commentedThis is shaping up to be a great patch. But:
I don't understand why "region" should be included in the vertical tabs with the others. It is a much more commonly used setting, and one of the fundamental things people do when they go to create or edit a block. This seems to go against the purpose of vertical tabs that was described by Bojhan and others above. @sun's suggestion in #35 of making it an uncollapsible fieldset makes much more sense.
Wording issues:
The suggestion in #35 of putting Roles after Content Types also seems worth doing.
Comment #44
sunI implemented the suggestions by David. And, yeah, also my earlier suggestion to keep the region settings out of the vertical tabs.
However, when using custom summaries for each tab, i.e. "Not restricted by role." instead of just "Not restricted.", then it is visually much harder to see a difference between the summaries. I think the purpose of the fieldset summaries should be to quickly see in which tab a non-default configuration exists.
Tinkering about this some more, how about not outputting a summary in case there was nothing configured?
Either way would work for me. Though I could see how "Not restricted." could be helpful for novice users.
Comment #45
sunAnyone?
Comment #46
mcrittenden CreditAttribution: mcrittenden commentedSubscribe. +1 on the concept and screenshots.
Small nitpick: for the "Page" setting, let's change "specified below" to just "specified" and "if the following php code...." to "if the specified php code". "Below" and "Following" only really apply when they're written directly above the options, not to the side.;
Comment #47
sunoh, the screenshots are outdated. The new summaries for "Pages" is:
...and this status is solely determined by whether there is a value in the textarea. Tricky, eh? ;)
Actually, without verifying, but this widget/fieldset most likely also needs an #elemenent_validate handler to ensure that a path was entered in the textarea when the "Show block on specific paths" option was selected. But that's a different/separate issue.
I recommend testing this patch live, because screenshots aren't very good in showcasing different interaction states.
This review is powered by Dreditor.
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedReviewed the code again and everything looks great. I was about to mark this RTBC but then trying it out, I found that the vertical tab summaries for roles and content types no longer work correctly (they don't update at all, regardless of which boxes you check). Probably not that hard of a fix, but I don't have time to look into it at the moment....
Once that's fixed, this should be RTBC in my opinion.
Also I'm attaching a screenshot of the page settings for those who are interested - although indeed, to really experience this patch it's necessary to try it out.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, very very very minor, but I believe elsewhere we don't use periods at the end of the vertical tab summaries (e.g., they should all be like "Not restricted by page" rather than "Not restricted by page.")
Comment #50
sunyeah, form element labels for radios and checkboxes have changed in the markup. Fixed that and removed the trailing periods.
RTBC according to David.
Comment #51
yoroy CreditAttribution: yoroy commentedCan we have a screenshot please?
Comment #52
sunWhat's wrong with the screenshot David posted?
But as mentioned before, better apply the patch and look at it live.
Comment #53
janusman CreditAttribution: janusman commentedSomething broken; picking values in the Content Types and Roles tabs shows nothing (or just periods) on the tab:
Comment #54
sunerrrr... did you test the latest patch on latest Drupal HEAD and did you clear your browser's cache? This sounds and looks like one of the three conditions is not met.
Comment #55
sunI seriously think that this raised issue is bogus, because I tested the latest patch manually. So back to RTBC.
Comment #56
RobLoachConfirmed. I tested it today and didn't run into any problems. RTBC * 2. Janus, make sure you clear both your server cache and your browser cache.
Comment #57
yoroy CreditAttribution: yoroy commentedSorry, overlooked the screenshot that was already posted above. Applied the patch and I'm reviewing it, already found some issues with the interface text changes. Posting full review shortly.
Comment #58
yoroy CreditAttribution: yoroy commentedThe settings behind the 'Pages' tab start mentioning paths instead of pages. This makes me think. Don't make me think. Yet. Suggest to rename the label back to 'show block on specific pages'. Maybe the description below the path text area can help make the connection between pages and paths more clear. "Specify pages by using their paths. …"
The summaries for the 'no restrictions' state are repeating the labels ( '…by page, …by role' etc). This seems redundant.
- Show for content types? Not sure 'for' is the best word here. Show along content types? Show with content types?
- The description still uses 'post' where we now want to use 'content' or 'piece of content'. Not introduced by this patch but we should fix it. First sentence could be reworded less formally as "Show this block only on pages that display content of the given type(s)."
- Since these settings are a mix of settings that either exclude or include visibility through making a selection, it makes sense to have the summary say 'Show on: (type), (type)' here.
- Another 'broken promise'. The tab mentions 'Users' but this is not confirmed by the label. You are making me think again! :)
Maybe 'Visibility configurable by the user'? Not pretty but a lot more specific. Open to suggestions this one.
- The summary 'Not configurable *per* user' still sounds like a top-down setting that isn't in the hands of the user herself but some admin specific per-user setting. 'Not configurable *by the* user would make this more explicit.
- Remove the comma in the 'Show this block by default…' line.
Comment #59
sunEat this.
Comment #60
Dries CreditAttribution: Dries commentedWould be great to get sign off from @yoroy on the latest patch. Good idea? Bad idea? Meets your bar, yoroy?
Comment #61
sun@yoroy: I am still thinking about the entirely-reversed-logic idea you raised. We should discuss that in a separate issue, because this one is really about using vertical tabs only. :)
Comment #62
yoroy CreditAttribution: yoroy commented'Users' tab is ok now. Customizable is an ugly word, but it is what it is.
'Pages' tab had a spelling error and could do with one more tweak.
Tried to fix these in attached patch. Otherwise looking good. RTBC if this shows up green
One more observation: Bojhan and me both were not a fan of this proposal. It's been good to see you guys making your own design decisions, specifically in keeping the 'regions' setting out of this. With only the visibility settings grouped together we think this works now. Good work.
@sun: that's why I only mentioned it in IRC :)
Comment #63
sunYay! Thanks, yoroy! :)
Comment #64
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #65
David_Rothstein CreditAttribution: David_Rothstein commentedThis really came out great - a joy to play around with.
Trying it out again, I have a nitpick with the page visibility options - makes a good followup (although some of the problems weren't caused by this issue):
Before:
After:
Hopefully a fair amount cleaner now?
Comment #66
sunI'd rename the first from "Every page" to "All pages", which I apparently did earlier, but yoroy reverted. The idea was to have the option's effect in the first word and also use "pages" (plural) for each:
- All pages
- Only ... pages
Furthermore, while discussing this entire issue/patch with yoroy in IRC yesterday, I had a monster WTF moment:
Why is a PHP-code-driven visibility setting intermixed with paths at all?
Comment #67
David_Rothstein CreditAttribution: David_Rothstein commentedDone.
Interesting question. I'm guessing because if it were a separate setting, then it's not clear which of the two would take precedence... on the other hand, we have that exact same issue already with paths vs content types, and the world hasn't ended yet.
Probably that should be for a separate patch, but it does sound like having "Advanced (PHP)" or what not as as a separate tab would make a fair amount of sense!
Comment #68
sunComment #69
catchJust a note on PHP, they're all stored the same and evaluated based on the radio option, it's not pretty in there - but separate issue entirely.
Comment #70
sunJust a note on the already committed patch: This should NOT have worked at all and is caused by an entirely broken implementation of vertical tabs.
I revamped the whole implementation and also fixed this wrong #group assignment in #657668: Vertical tabs break Form API
Comment #71
webchickCommitted #67 to HEAD. This was just string clean-up and unrelated to sun's comment in #70.
I presume this is fixed now, since the "vertical tabs are entirely broken" issue (?!?) is being dealt with separately.