Updated: Comment #0

Problem/Motivation

The current place block form requires the theme be passed to it ('/admin/structure/block/add/{plugin_id}/{theme}')
This prevents other places in Drupal from easily providing a link to adding a new block, because they would have to handle the theme picking logic, and worse, the UI for it.

Proposed resolution

Allow the theme to be NULL, and provide a select list.

Remaining tasks

Write this, probably tomorrow :)

User interface changes

N/A, but eventually other links to placing blocks could result in a form with a theme select list

API changes

The theme param for the place block route will become optional

Files: 
CommentFileSizeAuthor
#18 Screen Shot 2013-09-08 at 3.28.01 PM.png110.05 KBtim.plunkett
#18 Screen Shot 2013-09-08 at 3.27.59 PM.png110.37 KBtim.plunkett
#14 theme-selector-2083877-14.patch9.1 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]
#14 interdiff.txt967 bytestim.plunkett
#12 theme-selector-2083877-12.patch8.16 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#12 interdiff.txt3.14 KBtim.plunkett
#9 theme-selector-2083877-9.patch6.75 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,966 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#9 interdiff.txt3.22 KBtim.plunkett
#7 block-2083877-7-FAIL.patch3.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,519 pass(es), 180 fail(s), and 70 exception(s).
[ View ]
#7 block-2083877-7-PASS.patch4.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,891 pass(es).
[ View ]
#7 interdiff.txt2.28 KBtim.plunkett
#4 block-2083877-4-FAIL.patch1011 bytestim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,863 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#4 block-2083877-4-PASS.patch3.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]
#3 interdiff.txt1.38 KBtim.plunkett
#3 block-2083877-3.patch2.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#1 block-2083877-1.patch2.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Comments

Status:Active» Needs review
Issue tags:-Usability, -sprint, -Spark
StatusFileSize
new2.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,560 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Something like this.
Though it needs a whole ajax thing to switch the regions when the theme selector is switched.

Status:Needs review» Needs work

The last submitted patch, block-2083877-1.patch, failed testing.

Issue tags:+Needs tests
StatusFileSize
new2.8 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.38 KB

Here's the ajax part.

Issue tags:-Needs tests
StatusFileSize
new3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,845 pass(es).
[ View ]
new1011 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,863 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Status:Needs work» Needs review

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -228,6 +228,32 @@ public function form(array $form, array &$form_state) {
+        '#ajax' => array(
+          'callback' => array($this, 'themeSwitch'),
+          'wrapper' => 'edit-block-region-wrapper',
+         ),
The last line is indented an extra space.

This looks good and is a nice addition. I'll leave as NR in case someone else wants to test since it was only posted today but it's RTBC for me.

StatusFileSize
new2.28 KB
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,891 pass(es).
[ View ]
new3.96 KB
FAILED: [[SimpleTest]]: [MySQL] 56,519 pass(es), 180 fail(s), and 70 exception(s).
[ View ]

Expanded the test coverage, it didn't actually work before...
And fixed the indentation.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -228,6 +228,32 @@ public function form(array $form, array &$form_state) {
+      $theme = \Drupal::config('system.theme')->get('default');

Injection.

StatusFileSize
new3.22 KB
new6.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,966 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

The test added to the patch just adds block to default theme. Do you think we need to check it for other theme and after saving it redirects to proper page?

Status:Needs review» Needs work

The last submitted patch, theme-selector-2083877-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
new8.16 KB
FAILED: [[SimpleTest]]: [MySQL] 59,026 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Okay, but then we need to fix assertUrl too.

Status:Needs review» Needs work

The last submitted patch, theme-selector-2083877-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new967 bytes
new9.1 KB
PASSED: [[SimpleTest]]: [MySQL] 58,538 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

@tim.plunkett thank you for the changes. This looks perfect to me other then assertUrl changes. I have no comments about it. RTBC for me if green.

Not sure if this adds UI elements or not. Issue summary says no, but the whole topic makes it sound like yes. Tested this with simplytest.me but couldn't find a theme select list. Please clarify?

@yoroy As far I understand you need to have several themes enabled. Unless there is a D8 theme available, I guess you cant test this easily.

It is visible in core, but nothing links to it yet.

From the block layout page, all of the place block links result in a URL with the theme name appended
admin/structure/block/add/system_main_block/bartik
Screen Shot 2013-09-08 at 3.28.01 PM.png

But if you manually navigate to the URL minus the theme name, you will see the theme selector
admin/structure/block/add/system_main_block
Screen Shot 2013-09-08 at 3.27.59 PM.png

As I said, this is unused so far because the blocks page is theme specific. But for issues like #2083871: Allow menu blocks to be placed from the menu page, we'll need a way to open the block modal without predetermining which theme to use.

Screenshots help, thank you.

Status:Reviewed & tested by the community» Fixed
Issue tags:+Needs followup

Awesome, thanks so much for the screenshots. Very helpful. Reviewed this in IRC w/ Tim, and looks good to me.

Committed and pushed to 8.x. Thanks!

The only minor nit I could pick out is that in the case of a site using only a single theme (e.g. Minimal profile), you'll end up with a dropdown with a single value in it. It's not worth holding up a critical to fix that minor edge-case UX bug, but can we spin off a Novice task to take care of that?

Automatically closed -- issue fixed for 2 weeks with no activity.