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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Usability, -sprint, -Spark
FileSize
2.19 KB

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.

tim.plunkett’s picture

Issue tags: +Needs tests
FileSize
2.8 KB
1.38 KB

Here's the ajax part.

tim.plunkett’s picture

Issue tags: -Needs tests
FileSize
3.78 KB
1011 bytes
tim.plunkett’s picture

Status: Needs work » Needs review
benjy’s picture

+++ 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.

tim.plunkett’s picture

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

jibran’s picture

+++ 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.

tim.plunkett’s picture

jibran’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
8.16 KB

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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
967 bytes
9.1 KB
jibran’s picture

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.

yoroy’s picture

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?

Bojhan’s picture

@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.

tim.plunkett’s picture

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.

yoroy’s picture

Screenshots help, thank you.

webchick’s picture

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?

tim.plunkett’s picture

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

klonos’s picture