Posted by jackracker on July 9, 2009 at 3:34am
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user interface text |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | needs work |
| Issue tags: | block, Needs tests, Usability |
Issue Summary
It might be helpful to have a fourth radio button that simply said "Show on all pages". I know you can duplicate that functionality easily enough, but as a complete n00b, I clicked a radio button, freaked out that it would mean I wouldn't be able to see my block on my testing pages, and then had to investigate to find out what the options actually did, which led me here. Good learning experience, but it could have waited until later if there'd only been an obvious way to turn off the page restrictions.
Do all the options show all pages if you don't enter anything in the text field? Or does "show only on the listed pages" mean the block doesn't appear anywhere?
Comments
#1
I saw this suggested somewhere else as well. I think this is a good one.
#2
Marking this critical hoping someone will pickup, it seems to set off a lot of first time users - thinking they HAVE to pick something.
#3
Tagging.
#4
Working on a patch now. Since we'll now have four different visibility settings, it would be a lot better if we defined constants for these options now instead of magical comparisons to integers. Also, I had a crazy idea to hide the 'pages' textarea if the 'always show' option is selected. I'll need a usability review on this since I'm changing (minorly) the work flow for page visibility.
Also, the block page visibility code in block_admin_configure() is horrid.
#5
Still needs some work plus some tests. Plus if a jQuery guru could take a look at the additions to block.js, that would be awesome.
#6
Initial screenshots
#7
Usability wise, this is looking good to me.
#8
I would remove the word "below", since there is no "below" showing initially.
Also, fixing version.
#9
Changing title, still critical issue.
#10
I have tested this Dave's patch and it works well.
I have removed the word below as webchick's request and re-rolled the patch.
#11
Can I have your attention for http://drupal.org/node/556390 ? It uses vertical tabs for block visibility.
#12
If it passes, this is RTBC.
#13
Sentences need to start with a capital letter for consistency.
#14
rerolled with caps.
#15
Wondering ... because the text says 'below' and reference something which the user can't see, it feels slightly better to always show the text area but to just grey it out when you select 'On every page'. Thoughts?
#16
There we go, On every page - is now the default.
#17
why did it change status? ugh
#18
The last submitted patch failed testing.
#19
I think that the "On every page." setting should be faked on the back-end so that it is rendered as chosen when the user chooses "On every page except..." but leaves "Pages" blank.
I think that might make more sense to users. If I were to click "On every page except..." but leave "Pages" blank, I'm really choosing "On every page." So, after form submission, shouldn't the choice be shown as "On every page."? It doesn't really make sense to me to have one setting be shown two different ways.
This would also make "On every page." be the default selection.
I can fix up the patch tomorrow evening if I get some feedback.
#20
#21
path applied w/message:
(Stripping trailing CRs from patch.)
patching file modules/block/block.admin.inc
(Stripping trailing CRs from patch.)
patching file modules/block/block.js
(Stripping trailing CRs from patch.)
patching file modules/block/block.module
tested it and the configuration options/interface work well and make sense to me.
but when i go to add a new block (/admin/structure/block/add), i had the following errors:
* Notice: Undefined index: pages in block_admin_configure() (line 249 of /Users/arianek/Sites/drupal7/modules/block/block.admin.inc).
* Notice: Undefined index: custom in block_admin_configure() (line 313 of /Users/arianek/Sites/drupal7/modules/block/block.admin.inc).
AND if i select option 2 (On every page except those specified.) and save it, it works but when i come back to that block's config page, option 1 (On every page.) is now selected (perhaps the default is resetting?).
setting option 3 (On only the pages specified.) remained after navigating away and back to the block config, so works fine, it is just option 2 that is affected.
#22
#23
#24
The last submitted patch failed testing.
#25
This should fix the patch and apply to today's HEAD.
The patch previously failed testing because it introduced a new database value for block visibility, but did not change the code which determines whether a block should be considered visible. I've made the patch change the user interface but store no new database values so that the visibility determination code need not change.
#26
The last submitted patch failed testing.
#27
OK. It seems my last attempt failed testing because of an assumption in block.test that the default option for block visibility is 0, i.e. "On every page except those specified." This is no longer true (which is the point of this patch), so I've rolled in an add-one-line update to block.test (line 126):
<?php$edit['visibility'] = '0'; // 'On every page except...'
?>
This test having failed raises the question -- will changing the default visibility selection to "On every page" break some code? AFAICT, it would only break code that did automated POSTs to the block add or configuration pages without explicitly setting the visibility field.
#28
The latest patch doesn't work.
It stops JavaScript working on the block settings page.
Apart from that, it seems to work as I would expect.
#29
i didn't see any response to my comments in #21 - has this been fixed?
#30
@arianek: I don't get the error you described in #21. However, if you select "On every page except those specified" and you don't specify anything, the selection will indeed revert to "On every page" -- that is intended.
Apologies for the JS error. I had removed a small bit in tableDrag that seemed unrelated to this issue (whoops).
This fixes the JavaScript error in last patch version which caused JS not to work at all on a block's configuration page. (Also re-rolled to apply to current head.)
#31
Has anybody tried the latest patch? Is it possible for this to make it into D7?
#32
Confirming that patch in #30 works. Thanks adamdicarlo
I tested the following.
-Creating new block. Display on all page.
-Select "On every page except those specified".
-Select "On only the pages specified".
-Delete block.
-Issue in #21 is fixed.
Tested with.
-Drupal 7 (September 9, 2009 - 05:11) fresh install.
-Garland theme.
#33
Bojhan asked for my 2 cents. I think this is a good change but the wording of the last two options needs some work. They tell me I can 'specify' things but don't tell me how, so currently the screenshot in #32 looks like a bug, since there is no textfield.
I'm all for progressive revealing of info, but the labels need to be more clear that that's what's going to happen.
#34
From feedback received from webchick I am moving this back to needs work. The other two radio's now lack context, since they refer to a text area that is not visually there anymore. Lets work on making this apparent.
#35
@webchick: When looking at #32 screenshot it does look like a bug. But when looking and using the Drupal site it doesn't look like a bug. Because the textfield is dynamic. I mean it is hidden when option 1 is selected. And display when end user clicks on option 2 or 3. When clicking any of the option the wording is self explain. Plus end user gets more information below the textfield when option 2 or 3 is selected. Find below screenshots to clarify (option 1, 2, 3). To avoid confusion for the next issues I'll attached screenshot of all options.
Another way would be when option 1 is selected to disable and fade out the textfield. Find attached mockup to clarify. But I see two down sides to that. First, end user might try clicking on the fade out disabled textfield then think it bugs. Sometime end users are interesting creatures from another world who click on everything that seems clickable ;) Second for better usability less item on a page = better usability. Displaying item only when needed = better usability.
My vote goes for leaving patch in #30 as is.
@all: What do you think?
#36
Following #35 setting status to 'reviewed & tested by the community' so it can be notice by webchick or someone else. And maybe ported.
#37
I think that the suggestion in #35 for a greyed out field in option 1 is actually a really good solution. Anyone around who can make such a thing happen (I'm not advanced enough) - I don't think webchick will commit this before something more clear is there UI wise.
#38
But then consider the scenario where you have added some pages to the text field and switch back to 'show on all pages'. Greyed out text area that still shows the pages that were added to it? Now it would communicate that these items are 'locked'. Maybe edge case, but would definately be confusing. Just remove the text area content then?
Another approach is rewriting the labels for the last 2 radios:
o On every page except those specified (show list)
o On only the pages specified (show list)
which is a bit ugly repetitive, but could behave similar to the machine name pattern. not sure if this link should also actually select the corresponding label. Maybe not a link at all but just tell people it will be all right if you click this:
o On every page except those specified (will show a list)
o On only the pages specified (will show a list)
#39
That seems really clunky (repetitive and not very intuitive especially for newer users) to me...
Doesn't greyed out w/radio button unselected imply that though the content entered is saved it is inactive? It seems pretty logical to me that one would not expect the pages in a deactivated list to be affecting anything.
#40
Anyone worked on this recently?
#41
After reading this issue, I realized that comment #33 by webchick would be resolved by abstracting the phrases. So I changed the patch from #30 to reflect this.
The wording is now:
This IMHO connects well with the help text below the text field that appears:
"Enter one page per line as Drupal paths. The '*' character is a wildcard. Example paths ..... " etc.
Needs review!
#42
The last submitted patch, 514256-block-visibility-D7-41.patch, failed testing.
#43
Hum... I'm guessing this is connected to issue #556390: Vertical Tabs on block visibility settings...
I have no ideia how to solve this...
#44
Marking this has Normal since this does not break the site.
#45