Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#18 | Screen Shot 2013-09-08 at 3.28.01 PM.png | 110.05 KB | tim.plunkett |
#18 | Screen Shot 2013-09-08 at 3.27.59 PM.png | 110.37 KB | tim.plunkett |
#14 | theme-selector-2083877-14.patch | 9.1 KB | tim.plunkett |
#14 | interdiff.txt | 967 bytes | tim.plunkett |
#12 | theme-selector-2083877-12.patch | 8.16 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettSomething like this.
Though it needs a whole ajax thing to switch the regions when the theme selector is switched.
Comment #3
tim.plunkettHere's the ajax part.
Comment #4
tim.plunkettComment #5
tim.plunkettComment #6
benjy CreditAttribution: benjy commentedThe 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.
Comment #7
tim.plunkettExpanded the test coverage, it didn't actually work before...
And fixed the indentation.
Comment #8
jibranInjection.
Comment #9
tim.plunkettComment #10
jibranThe 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?
Comment #12
tim.plunkettOkay, but then we need to fix assertUrl too.
Comment #14
tim.plunkettComment #15
jibran@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.
Comment #16
yoroy CreditAttribution: yoroy commentedNot 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?
Comment #17
Bojhan CreditAttribution: Bojhan commented@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.
Comment #18
tim.plunkettIt 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
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
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.
Comment #19
yoroy CreditAttribution: yoroy commentedScreenshots help, thank you.
Comment #20
webchickAwesome, 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?
Comment #21
tim.plunkettOpened #2087207: Only show the theme selector on the block form if more than one theme is enabled!
Comment #23
klonos