Problem/Motivation
During usability testing, there wasn't a single participant who was able to successfully place an existing block. The right sidebar is dead to them. They're taught this both from the new add content page, but also from the entire internet at large, where the right sidebar is mostly either supplementary links or ads (aka, "ignore this").
Typically, participants would start by 1) scanning the massive table below (some looking for an add button/link in the region table rows), 2) going to the Add block button looking for a way to add dynamic block content from there, and finally 3) hitting the "Custom block library" tab:
However, this was not helpful either, because it's actually a list of only your custom blocks, *not* a "library" of anything. I think the quote from one participant was something like, "Custom block library listing, now we're talking! ...Wait. I thought you said you had a block library?!"
I know we typically do not make UX problems critical, but IMO this one is. It's a primary interaction of a primary site builder UI and it represents a significant regression from D7.
Proposed resolution
Several approaches to fixing this have been debated and worked on at various points in the issue's life cycle.
The original proposed resolution (in #17, reflected in the patch at #207) consisted of removing the sidebar, moving the block library to its own page, and making "Place block" the primary action on admin/structure/block page, moving "Add custom block" to a sub-tab.
However, there were concerns raised that:
- It moves the block “placement” further away from the blocks layout page, which was an important goal and achievement of the sidebar design.
- It still leaves people looking at the region and then going back to “add action” to place it, versus putting the "add action" where it's more contextually relevant (on the region itself).
- The custom blocks add interaction becomes much more buried, making it harder to discover.
To address these concerns, a new proposed resolution was brainstormed in #210 by @tim.plunkett (Block system maintainer), @Bojhan (UX maintainer), and @webchick (Product manager):
Mockup 1: Remove the sidebar, in favour of a "Place block" button in each region's table row.
Styled after the Views UI's similarly contextual "Add" buttons.
Also remove "Add custom block" as the primary action on this page. (In #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled we can make "Demonstrate block regions" the primary action instead.)
Mockup 2: When clicked, open a modal with the block listing that's currently in the sidebar
...and put the "Add custom block" action button at the top of this page instead.
- Please pretend this is in modal, and the primary tabs/breadcrumbs are not present.
- Also, the buttons next to each row should say "Place block" vs. "Add block" to eliminate confusion.
Then, after choosing a block, you get the next modal with block configuration (region should be pre-populated).
Remaining tasks
Code it!
User interface changes
The stuff in proposed resolution.
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#263 | Screen Shot 2015-07-10 at 9.47.57 AM.png | 69.19 KB | webchick |
#259 | Block layout | Drupal-2015-07-10.png | 83.53 KB | tim.plunkett |
#259 | interdiff.txt | 421 bytes | tim.plunkett |
#259 | 2512456-block-259.patch | 41.41 KB | tim.plunkett |
#252 | interdiff.txt | 12.68 KB | tim.plunkett |
Comments
Comment #1
davidhernandezTo be clear, we are talking about this thing, right?
Comment #2
webchickThat's the one!
Comment #3
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedIf nothing else the wording is quite bad - a block instance is being created, not placed? The blocks in the center already exist and can be placed?
There is also a tab for the custom block library at the top. More WTF.
Comment #4
webchickWhat's interesting is they did at least find that tab, though it was usually the third thing they tried, after trying to make sense of the massive table below and then going to the Add block button. However, when you go there, it just contains a table that is totally empty. This is because it's actually a list of only your custom blocks, *not* a "library" of anything. I think the quote from one participant was something like, "Custom block library listing, now we're talking! ...Wait. I thought you said you had a block library?!"
Comment #5
tim.plunkettThe first half of #2078601: Move the block placement browser to the left of table and collapse it would make it more obvious, but collapsing it probably won't help.
Comment #6
webchickFurther fleshing out issue summary. I was originally going to split off the block library one to a separate issue but it's quite related to the one here.
Comment #7
webchickAh, thanks, adding that one too.
Comment #8
Bojhan CreditAttribution: Bojhan as a volunteer commentedAdding a bit of context. The core of the issue is that all of our users in the UMN 2015 test had significant difficulties in placing just a block. Since this is a key site building concept, that users had no difficulty with before - we are making this critical. UX issues such as this should be marked critical, as it severely hinders new and existing adoption of Drupal 8.
We identified several possible solutions. I will pick up and do some design exploration, I think there are multiple avenues to this. We need to ensure that, the time we put in is towards the most valuable one. I think either 2) or 3) would be most fruitful. While 1) is an option, I think the fact that it is considered supplementary will make people just go to the right side.
Comment #9
dawehnerMy quick thought, why don't we just add it also an explicit step to add blocks. This allows to explain users what is going on, which is IMHO of a higher value than making things easy.
1) sounds just like a workaround to be honest. 2) Seems to be the solution which is more common with the entirety of the Drupal UI. If you add something you have an add button
Comment #10
dawehner-
Comment #11
philsward CreditAttribution: philsward commentedIf it were me, I'd just add a third tab, placing it before the custom block tab, and call it something like "dynamic block library" or maybe "persistent block library". Verbage that denotes it as being a bit more static as opposed to being custom. Use the two library pages to enable/ disable which blocks are placed.
On a side note, "layout" should be renamed IMO. Layout is usually what happens after the item is placed. Rename it to "block placement"? Ideally you are placing the block on the page. It would fit the current verbage of placing the block. What happens when you throw panels or display suite into the mix which completely overrides all predefined themes? You now have block layout through core and layout through panels. To me, layout makes more sense when using a system like panels or DS. From a core standpoint, core doesn't (currently) handle the layout of anything. It simply regurgitates what it's told.
Installed on simplytestme and didn't even see the placement options until I scrolled to the very bottom of the region list. Lots of scrolling on mobile.
Comment #12
davidhernandezJust want to say that I'm really glad this is being address. When I first tried Drupal 8, this was the biggest WTF for me. I took quite a while to figure out how the block administration worked. In the list of proposed solutions, #2 sounds the best. It falls in line with how fields are added. You click an "add" button and the choose new or existing.
Comment #13
dawehnerYeah special case UIs are local optimizations which doesn't help you to find necessarily the global optimum.
Comment #14
cilefen CreditAttribution: cilefen commentedWhy don't we call that thing the "Block Library"?
Comment #15
Fabianx CreditAttribution: Fabianx as a volunteer commentedI agree with third tab, I myself just recently searched how to place a block myself ...
Block Placement | Block Library | Custom Blocks
Comment #16
MattA CreditAttribution: MattA commentedPreview of moving the sidebar to the left:
My suggestions:
Comment #17
Bojhan CreditAttribution: Bojhan as a volunteer commentedThanks Matt. Moving the sidebar over was also our first thought - as you throw it right in there face (hopefully). One of the things that does though - is creates a unique pattern to the blocks page (as we don't do this elsewhere). One of the things we noticed is that users are very attracted to the table of drag-and-drop.We fear that simply moving it, doesn't really solve the core of the problem.
We discussed this over the spriting at DrupalCamp Minneapolis (Webchick, Lewis, Ivan et al.). The solution to this problem is quite tricky, and there are very different ways to solving this.
From an interaction design perspective we wish to reduce the confusion by:
Users were unable to achieve this task because they did not see the sidebar. Users ventured into custom blocks without realising the fact that there are existing blocks that can be placed.
To achieve this we think the best way to tackle this is to take the following actions:
While this seems rather radical, this critical problem truly prevented people from learning a core concept of Drupal. While we can try various visual tricks - we ought to really streamline this experience with the rest of core.
The challenges we see are mostly in using the correct labeling and descriptions to describe the concepts. Key here is that users are not tempted into custom blocks, simply as a "more" pattern - but rather knowning its purpose. We have learned that venturing into the custom blocks interface, can be misleading them into thinking there are no other blocks.
Mockup 1: Only provide one "place" interaction on the block layout
Mockup 2: Allow users to select all kind of blocks (module provided, custom)
Mockup 3: Provide users with the block configuration page
Mockup 4: Provide a separate page, from where you can add the custom blocks (same as now, different labels)
Mockup 5: Provide a separate page from, where you can add block types (same as now, different labels)
I would love to know what you think, this is quite a tricky problem - in a space that we had much bigger plans for. However we now need to move forward to release, and this critical building block should be highly usable.
Comment #18
Bojhan CreditAttribution: Bojhan as a volunteer commentedComment #19
legolasboSince the overall consensus (2hours ago) seems to be in favour of option 2, I've started work on an implementation of said option.
Attached you'll find an initial path which removes the block placement bar and moves it to a separate page. I'll continue working on this to match @Bojhan's mockups.
Comment #20
MattA CreditAttribution: MattA commentedHmm... Well let's start with the mockups:
Mockup 1:
Has a major issue of no obvious way to let users create a new block. First thought was definitely not to look in the "block types" tab. As you mentioned, labeling is so important! It also has that "classic" layout. Previous Drupal users are going to expect disabled blocks at the bottom - are we keeping that section?
Mockup 2:
Well it works. If that button creates a modal dialog, it's fine, but if not it should really be a select list.
Mockup 3:
Not different from what we already have, but... "UGH. I have to visit that form for every block I want to move? No #@$%ing way." ...was my initial thought if it became required to moving blocks around.
Mockup 4/5:
Don't mind. Same as #1 for that tab label though.
The more that I think about it, the more I think that we might also be missing the obvious solution. What if we placed disabled blocks into "default" sections of the table right next to active blocks that would be greyed out and have an "enable" link? If that link was ajax-ified and worked like the edit field gear icon, and then allowed in-place configuration... could be promising. It might require changes to the block plugin system though.
Also, personally, I group blocks and themes together in my head for whatever reason. Like the theme page, I kind of expect a slightly unique UI/appearance on the block page. What if block layout was a child of the appearance menu? Would people be more willing to deal with a custom UI and let us get away with different concepts?
Comment #21
legolasboI'm using the mockups as a general guideline since I already anticipated disagreements on certain elements (such as Mockup1's tab label). Attached you'll find a new patch + interdiff + screenshots of the current add block workflow.
New block layout page.
I've chosen to rename the Block library tab to be Custom blocks because that's what the tab actually is, a way into custom block management. However, I feel this tab might actually be misplaced here.
New block placement page.
This is currently just the original sidebar, but moved to a seperate page. Will refactor this to more closely match the other admin screens.
Placement of a block. AJAX goodness
Nothing has changed here, Clicking one of the blocks will open a modal.
Submitting the modal adds the block and returns the user to the block layout page.
Comment #22
MattA CreditAttribution: MattA commentedHere's a preview of my thoughts in #20:
This way blocks are nice and visible right there in the main table, instead of buried at the bottom, if that's what we're going for. They can also be immediately enabled and set in the user's desired region, with ajax changing the link to a "configure" button. There's just that pesky issue of implementation...
Also, yes to getting rid of the regions column. It is redundant information and at odds with the concept of a draggable table.
Comment #23
legolasbo@MattA,
how does your proposition handle adding multiple instances of the same block? I'm afraid the list of uninstantiated blocks will clutter the block layout overview.
Personally I think separating the block placement from the block layout tremendously cleans up the admin UI at the cost of 1 extra click. Therefor i think option 2 is the best way to solve this issue.
Comment #24
legolasboComment #25
legolasboRemoved accidentally embedded images.
Comment #26
MattA CreditAttribution: MattA commentedIt doesn't, so scratch that idea. Ironically enough, I had removed the sidebar while working on the concept and forgot about that feature. /sigh
Comment #27
MattA CreditAttribution: MattA commentedOn a side note: how about "add block to layout" instead of "place block" to keep with standards?
Comment #28
dawehner@legolasbo
It is great to see progress here.
In general I think being able to search might still makes sense on that additional page. Do you think it should be part of a modal as well? The views UI has a similar pattern when you add a new field for example.
Comment #29
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThere is already a separate issue: #2513534: Remove the 'disabled' region from Block UI
I think we should be able to keep these two patches separate, which would reduce the complexity of this issue.
Comment #30
Bojhan CreditAttribution: Bojhan as a volunteer commented@dawehner Yhea, I'd love if we can retain that. I didn't want to assume that we can easily paste it over though - if we can that'd be great.
Comment #31
legolasboAnother update. The block placement page now actually respects the chosen theme in stead of just placing blocks on the default theme.
@dawehner I was also thinking that the search feature should be retained. As for placing the actual block placement form in a modal... I didn't quite think of that option, but that would be a great way to keep users informed of the context of the form. However, for now I'll focus on actually implementing the proposed resolution. We can always add the modal in a follow-up issue.
Comment #32
dawehnerTotally agree. I was just thinking out load.
@legolasbo++
Comment #33
StuartJNCC CreditAttribution: StuartJNCC commentedOne thing that has caught me out a few times when I have looked at this screen is that the items in the "Place blocks" list are prefixed with "+". In Drupal, that usually means you can drag and drop them - e.g. rearrange the order of items in a list. So, several times I have come to this screen and tried to drag a block from the "Place blocks" list and drop it in one of the screen regions on the left. Of course it doesn't work (wherever you drag it, it always has a "no entry sign" cursor) and hence evokes a momentary WTF? reaction.
When I think about it, I realise that the "+" in this context denotes an expanding/collapsing hierarchical list. But drag and drop does seem the natural way to go about it here.
Why not a drag and drop interface?
Comment #34
davidhernandez+1 regarding what StuartJNCC sad. The first time I used Drupal 8 that is exactly what I tried to do with the "+".
Comment #35
MattA CreditAttribution: MattA commented@StuartJNCC Yup, I've done that... as recently as today...
As mentioned in #17 from a usability perspective, keeping things consistent leads to people knowing what to do, generally speaking. So creating a sidebar that people may or may not see, and then adding a drag-and-drop behavior that isn't implied, mentioned, or used elsewhere in core isn't ideal.
@legolasbo Nice work so far. Just some nitpicking:
Should probably make the path: admin/structure/block/placement.
The "Add custom block" action on that page probably isn't necessary/optimal.
It might be better to also use the theme local task on that page too.
Comment #36
webchick#33/#34 are super interesting observations. Could you spin off a separate issue for them (against Seven theme)? I don't want to really touch styling as part of this since it's complex enough already.
And wow, @legolasbo++! So wonderful to see work being done on this already!!
Comment #37
legolasbo@MattA,
I agree with the nitpicks and will incorporate them.
For now I've gotten a little stuck, which is probably caused by working on this for too long. I'll continue work on this on thursday. Unassigning the issue in case someone else wants to pick up work on this in the mean time.
Comment #38
rickvug CreditAttribution: rickvug commentedThere is a lot of space beside each region title (Header, Primary Menu, Secondary Menu, Help, Highlighted). What about a "add block" link directly beside these titles? From there open up a modal to select the specific block or add another. My gut (needs to be validated) is that users think layout first when wanting to add a new block to a page.
Comment #39
Fabianx CreditAttribution: Fabianx as a volunteer commented#34: Yes, count me into the users that tried to drag and drop the blocks ...
Comment #40
webchick#38 is a great suggestion, and another issue that was identified during this round of UX testing. #2513528: Add a link to add a block in empty regions on the Block layout page.
Comment #41
MattA CreditAttribution: MattA commentedTook a fresh stab at this just to get more familiar with Drupal...
@todo
- Tests. Duh.
- Add/remove/edit help text.
- Thoroughly review the javascript, I haven't written code in that language in years...
- I might need to reinstall, but the Custom blocks page title disappears after the first time viewing it?!?!
- There's quite a bit of unused CSS laying around now.
- The grey modal background doesn't play nice with sticky headers.
Comment #42
legolasbo@MattA,
I wanted to take a look at your patch to see which direction you are taking, but unfortunately it failed to apply.
Comment #43
MattA CreditAttribution: MattA commentedHmm... ok, tried it myself, and after some quick Google-ing it seems PowerShell messes up the encoding. Seems it output in UTF-16?! That would definitely explain the large file sizes of my last couple patches...
Let's try this again, not in Windows this time. :D
Comment #44
MattA CreditAttribution: MattA commentedNope, missed the new files. /sigh
Comment #45
Manjit.SinghComment #49
darol100 CreditAttribution: darol100 as a volunteer and commentedI'm not sure why the automatic testing is falling. However, I notice that patch from #44 have a lot trailing whitespace. So here is an updated version of #44 without those trailing whitespace.
Comment #51
tkoleary CreditAttribution: tkoleary at Acquia commentedPosted this design over on #2513528: Add a link to add a block in empty regions on the Block layout page. the block list expanding is a related but additional to that issue.
Comment #52
tkoleary CreditAttribution: tkoleary at Acquia commentedAfter looking at this for a while I think maybe the block list needs to be labeled with a verb to underscore the action required. But instead of "place blocks" I'd say "Choose a block"
Comment #53
LewisNyman CreditAttribution: LewisNyman at Wunder commented@tkoleary That's pretty cool! I feel safer if we went with a modal though, if we aren't aiming for a drag-and-drop interface in this release. A modal is impossible to miss :)
Comment #54
MattA CreditAttribution: MattA commented@LewisNyman Yes, modal is definitely the way to go if we do this. My current dev site has it, and it turns the whole process into a nice ajax wizard like effect. :)
There are quite a few options when it comes to adding header links though.
Personally, I don't feel like any of them look great, but for now it is probably all we can do. I am more a fan of Plan B (preferably with drag & drop support and weight handles).
Comment #55
MattA CreditAttribution: MattA commentedHere's a patch with the ajax goodness (went with option #3 just for simplicity).
Comment #56
legolasboEven though I like the idea of being able to add blocks to a region directly, I think this falls outside of the scope of this issue.
Let's focus on moving the sidebar to a separate page and address the direct block placement feature in #2513528: Add a link to add a block in empty regions on the Block layout page. after this issue lands.
Comment #57
Fabianx CreditAttribution: Fabianx as a volunteer commented#56: "Users were unable to figure out how to place a block" - I think the patches and screenshots give users easy ways to place a block?
Comment #58
legolasboI'll work on the patch in #49 for a few hours to figure out why the tests are failing.
Comment #59
MattA CreditAttribution: MattA commentedYeah the more I use it, the more I feel that it just doesn't look right (the workflow is so much better though!). Oh... and the table sorting doesn't work in a modal dialog. I don't even know where to begin debugging that issue though, so just going with #44 and saving that for a follow-up sounds like a good plan for now.
Comment #60
LewisNyman CreditAttribution: LewisNyman at Wunder commentedPlease do try and keep the patches as simple as possible but keeping solutions to their separate issues. It get's harder to review a patch if it introduces loads of changes.
We already have these two issues: #2513528: Add a link to add a block in empty regions on the Block layout page. #2513520: Add a contextual link to add a block to a region
Comment #61
legolasboI agree with @LewisNyman,
The changes in #44 already broke 67 assertions which need mending. Adding additional functionality would add further complexity to this already growing patch.
I'll upload a new patch which fixes several broken tests in a few minutes. There are still a few to go, but I'm running out of time for today.
Comment #62
legolasboAttached patch fixes several tests which were broken in #44. I'll continue work on this tomorrow.
Details for each test are listed below:
Drupal\block\Tests\BlockLanguageCacheTest
was failing because we moved the block listing. After changing the paths it passes again.Drupal\block\Tests\BlockUiTest
was failing because we moved the block listing and changed the listing to a table. After changing the paths and xpath queries it passes again.Drupal\block\Tests\BlockXssTest
was failing because we moved the block listing and changed the listing. Adjusted the test accordingly.Drupal\block_content\Tests\BlockContentListTest
was failing because of the changed content listing title. I’ve changed the test accordingly.Drupal\block_content\Tests\BlockContentTypeTest
was failing because it was testing if the user would be redirected to the block configuration form after a custom block was added from the block placement form. Removing the Add custom block button is actually a major regression because users would now have to follow the following path to add a new custom block to the layout:I’ve undone the removal of the Add custom block button and relabelled it to Add new custom block to layout. After which I've changed the paths in the tests to reflect the new block placement url.
Comment #64
MattA CreditAttribution: MattA commentedHmm, I still have strong reservations about bringing back the custom block action button. Specifically the thoughts mentioned in #17:
Here is the train of thought that I think new users will go through for each button:
One solution is to explain what a custom block is, and provide a link to create one in the help text instead. That way we can still emphasize our desired primary interaction. This also goes well with efforts in #2514150: Advertise the block region demonstration in a more prominent way. So we effectively switch the demo and custom block positions and end up with:
Comment #65
MattA CreditAttribution: MattA commentedComment #66
legolasbo@MattA,
How about something like this?
That would still allow the user to directly add a custom block, but still emphasises the desired primary action.
Comment #67
MattA CreditAttribution: MattA commentedIt's a nice visual aid, but no, I don't think it is enough.
Some thoughts:
a) Visual aids don't help the color blind, or in tree testing.
b) From a language perspective, the two combined are still bad. The word "new" does nothing to clarify the action, as that is what both buttons do. The word "custom" is just too ambiguous as well. There's just no way to do what we want in one word. Maybe two if we used something like "user-defined" instead, but even that just makes it technical. Mind you, given what actually happens, the button's verb is also wrong. It should really be more like "create and add
block to layout" (also, the '+' symbol which implies "add" is set by the theme, and can't be removed).
c) No where else in core is there a grey action button. Not sure if we should start a trend here.
d) Even if we wanted to, I've looked into the code, and it doesn't look like we can easily change the button color since it is apparently hard-coded into template_preprocess_menu_local_action() as
$link ['localized_options']['set_active_class'] = TRUE
.Comment #68
MattA CreditAttribution: MattA commentede) Wait, why are we even discussing this! We're getting side-tracked. This issue should just be about making the block placement action obvious and moving the "custom blocks" link to a help text paragraph certainly does that (while not causing a total regression of the old custom block workflow). If we need to make it more prominent later then we can create a new issue and let some other guy worry about it.
Comment #69
legolasbo@MattA,
Good point in e). let's move into that direction. Fixing/closing this critical should be our first priority. Removing the ability to create & place a custom block would introduce a new regression. Moving the link to do so however does not introduce said regression while still focussing on improving the block placement/layout workflow.
Comment #70
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThe current patch is looking great! Good to see so much progress so quickly.
I'm a little concerned that we're moving a little too fast for some with less time to keep up. @Bojhan has yet to respond the feedback to his designs. It's important that we get buy-in from the UX maintainers to make sure we are designing a consistent interaction with the rest of core and keeping other problems and concepts in mind.
At the sprint on Sunday we discussed some of the points that @MattA has raised in #20 and Bojhan's designs have already considered some of them. For example, the reason why the 'create custom block' button is hidden further down in the IA is because that button was extremely misleading
Could we please focus our time and energy in Bojhan's original designs for now? I think there are a few tweaks and improvements we can add to it, like adding the AJAX block settings and filtering, we should be very careful of overbaking the designs in one patch.
I've updated the issue summary with Bojhan's designs. Hopefully we can move forward with consensus.
Comment #71
MattA CreditAttribution: MattA commentedLet's see how this goes...
Notes:
block_admin_display_form
?Depending on how this turns out, our remaining tasks would be to:
Comment #73
dawehnerI gave #71 a try. It looks pretty great and works as expected for me. Looks like the minimal amount of changes here to get the critical issue fixed.
Great work so far!
When manually testing the patch I thought it might be better to sort by category by default, so the behaviour is more similar to before ... Given that I am not sure whether its worth to be able to choose which click sort you want.
Looks perfect for me
Wow, that xpath looks really detail, maybe we can figure out something better which also doesn't break too easy, in case the HTML would change. For example maybe it is enough to check for the a href with :href and nothing else.
Yeah its a good question whether just doing that would be enough, it seems that this could lead to another level of confusion, as its not trivial to find where to add a custom block.
Well I think there is some value in checking that the labels on the extra page are sanitized properly as well.
Good question. In general yes, I think this should live in
block_content</block>. Regarding <code>hook_help()
vs.hook_form_alter()
I would lean towards the first approach, so you can disable help module, in case you don't need that.
Comment #74
lauriiiI tested the patch manually and it actually works better than the test fails let me expect ;) I guess most of the test fails are because web tests are failing to set blocks into regions.
Just a few nits from the patch but overall looks good.
Maybe use $this->t instead
@return mixed[] is little bit more explicit
Maybe use $this->t instead
This is a little bit odd. I don't know why it is like this but maybe it could be done some other way.
Comment #76
legolasboI Queued the patch for retesting to see which tests actually fail. I'll continue work on this 2 or 3 hours from now.
Comment #77
MattA CreditAttribution: MattA commented@dawehner
#3: The href is being used to make sure it's the right block since like 3 of them have the same name. The test also specifically changes the category so that part needs to be checked too. So as far as I can remember, only the text is optional.
#4: Before deciding, note that it is kind of similar to #2513580 in that a functional link should not be part of help text.
@legolasbo Looks like it was just the testbot. Greeeen! :)
This should be wrapped in t() though. Don't know how that even happened.
Comment #78
MattA CreditAttribution: MattA commentedSo I'm pretty sure this would work:
$pattern = '//tr[.//td[text()=:category] and .//td//a[contains(@href, :href)]]';
Don't know why I didn't just do that to begin with... should also let you remove the ':text' field from $arguments as well.
Comment #79
legolasboStarting work on the notes in #73, #74 and #77
Comment #80
dasjoNote that xano has written a "Plugin Selector" which aims to be a reusable solution (think views add dialog) for drupal 8:
https://www.drupal.org/project/plugin
Its probably out of scope to implement this here but as the current views add dialog implementation already lacks re-usability, I would like to propose that the mid-term solution should be something reusable. We'll need the same for adding rules configurations for #d8rules for example.
Comment #81
legolasboI've gone over the issues mentioned to start with a clean slate before further development and will continue with
Notes
#73-1 Category is now the default sort column.
#73-3 see #78 below.
#73-4
This was my motivation to adjust the test instead of removing it.
#73-4 #77-4 We should discuss this further, but I don’t think the discussion should hinder the development of this patch since block placement is the main focus of this issue. In my opinion, the only thing we should discuss in this issue is if the removal of the option to add a new custom block from the block placement form is an acceptable regression or not.
#74-1 replaced all t() calls in BlockListController by $this->t()
#74-2
Is from EntityListBuilderInterface and also encountered at other methods returning a render array. It seems to be a convention to document returned render arrays as an array.
#74-3 see #74-1
#74-4 @lauriii Could you clarify what seems odd to you?
#77 Wrapped in $this->t()
#78 manually tested he xpath-query and it selects a unique and correct row. Adjusted the query accordingly.
Comment #82
legolasboOops
Comment #83
MattA CreditAttribution: MattA commentedComment #84
legolasboFixed #83
Fixed my xpath mistake from #81 and #82 and this time waited for a green test locally ;)
Removed unused CSS from
block.admin.css
by first locating all pages where said file was used and then searching for selectors to see if the css rules still apply. Also added some comments to the remaining sections to clarify why they exist.Comment #85
dawehner.
Comment #86
darol100 CreditAttribution: darol100 as a volunteer and commentedGreat work, but...
\ No newline at end of file
Comment #88
legolasboWhile thinking about a good way to formulate the custom blocks help paragraph i encountered 2 things.
Let's say i want to add a new block to the layout. My first instinct (even though i know I can't actually succeed that way) is to click "add block to layout". Which leads me to the new block placement page. That page only lists existing blocks and has no way to add new blocks nor does it offer an easy way to return to the regions listing as seen below.
This means I have to go back to square one and follow the successful path where I first click the "custom blocks" tab. On the new page I see a listing of custom blocks and a "Add custom block" button which I click. I can then fill out a form to add a new block.
So far so good, but upon clicking "Save" I return to the custom block listing.
This means I have to click the "Block layout" tab (if I use blocks often and know that gets me to the right page). And then use the steps described above to add the block to the layout.
To me this seems like a lot of (confusing) work. Especially for inexperienced users. And I think it is impossible to write a short paragraph to describe these steps. Even if we were to manage to explain these steps in a short paragraph it would be something like this
Basically i see two options here:
Comment #89
legolasboMy preference goes towards option 1. I think we should limit the scope of this issue to the removal of the sidebar in favour of a new block placement page. We can still address the custom blocks in a follow up issue.
Comment #90
dawehnerThis sounds fine for me ... we could even use destination here to redirect the user to the placement block page after adding the custom block.
Comment #92
legolasbo@dawehner,
Thats what the original implementation did.
Comment #93
darol100 CreditAttribution: darol100 as a volunteer and commented1+ like this idea, we should keep it simple for now, just like @LewisNyman mention in couple comments already.
Comment #94
mbrett5062 CreditAttribution: mbrett5062 commentedInstead of "Add custom block" for creating it, could we not rename that to "Create custom block"
So "Add" is used for placement, and "Create" is used for creation.
It seems a little less confusing.
I see this as Adding Blocks to the layout, including both supplied by core blocks and custom user created blocks.
Then you have the additional functionality of creating your own custom blocks for inclusion into the list of available blocks that can be added to the layout.
Comment #95
mbrett5062 CreditAttribution: mbrett5062 commentedAlso maybe the block listing/placement page/tab could be subdivided. Showing all blocks, but on collapsible fields for "Core Blocks" "Module Blocks" "User Blocks"
Comment #96
dawehnerGood point @mbrett5062!
It explains really better about the underlying architecture.
Such conversations should happen in a follow up.
Comment #97
legolasboSo, to summarise, remaining tasks:
Possible follow up issues:
Comment #98
lauriiiThis issue needs also new tests for the way block page works now
Comment #99
lauriiiGood work on the issue everyone and thanks for the IS update! We have very nice progress here. Some comments of the patch we have right now:
After removing this
use Drupal\Component\Serialization\Json;
can be removedAfter removing this
use Drupal\Core\Url;
can be removed too :)Ubernitpick but can we move this variable declaration before the foreach so its explicit that its being populated there?
This could use the new array syntax
(optional) change to have new array syntax :)
Comment #100
legolasboIt doesn't need new tests, because we've adjusted the tests according to the changes we've made. Unless you can pinpoint specific changes which are currently untested of course.
Comment #101
Bojhan CreditAttribution: Bojhan as a volunteer commentedThanks for bringing the scope back down to the original issue. That's really appreciated.
While the dialog interaction is nice - it is actually completely a hack. Views uses this pattern, but it has not consistently made its way throughout core. We added it to blocks, in the anticipation of rolling it out further - but never made it. I think for now we are safe to assume, that while its a nice pattern - we shouldn't pursue something that is half implemented across core.
Having a dialog in the block placement flow, makes little sense - you are not moving back to the page behind it. This should just be a normal page.
I understand the concerns with regards to adding a custom block. However I am adamant about not having two primary actions on the top of the page (just making the other gray, doesn't help). We have learned in testing Drupal 8 that having two action buttons is incredibly confusing, and this test again made it clear that people do not know what "adding a custom block" necessarily means. At best I would go for option 2) of adding it to the block placement page, however it is a bit of a break in the mental model. So I am not yet convinced.
Comment #102
MattA CreditAttribution: MattA commentedFixed a few help text issues:
and provides a <a href="!block-listing">Custom block listing</a> listing all of them.
Simplified some labels from "custom block listing" to just "custom blocks" where appropriate.
Updated with most of the changes from #99 (not #5).
Removed the ajax stuff as per #101.
@Bojhan I totally agree with not having another primary action, but I also don't think adding it to the new page we just created is the solution either. We might be stuck with just adding a sentence to the help text, but that's all for a separate issue...
Comment #104
MattA CreditAttribution: MattA commentedFixed that one test...
Comment #105
MattA CreditAttribution: MattA commentedComment #106
legolasbo@Bojhan, @MattA,
In your opinion, where does that leave us in regards to adding a custom block from this patch? I guess we'll just accept the regression for now?
Perhaps we should treat custom blocks as content in a same way the D7 bean module does. That would remove the tight coupling of the block and block_custom UI and possibly simplify maintenance. But that's a discussion for a follow up issue.
Comment #107
googletorp CreditAttribution: googletorp as a volunteer commentedFixed some code style issues and ESLint validation of block.admin.js.
Comment #108
legolasboChanged array notation for code touched by this patch to use the shorthand notation for consistency and removed an unused use statement.
Comment #109
legolasboComment #110
lauriiiAll the code we have in the patch looks sane and clean. We have still remaining task regarding the add custom block button which should be restored. Setting for NW to get that done.
Comment #111
legolasbo@lauriii,
This issue is about focussing the block layout page on it's primary task, which is the layout and placement of blocks. While creating and placing custom blocks has some relevance regarding this issue I think we should consider it out of scope for now.
We should open a follow-up issue (perhaps even a critical) to discuss and change the custom_block UI in general. Because besides this button, the "Custom blocks" tab also seems out of place here and when we start moving these things around, more issues will arise which would extend the scope even more. For now let's keep focussing on the changes at hand.
Comment #112
lauriiiI'm sorry I missed #101. Updating the remaining tasks then.
This looks good in all terms for me so I'm setting this to RTBC.
Comment #113
Bojhan CreditAttribution: Bojhan as a volunteer commentedLets allow for some review.
Comment #114
lauriiiJust a clarification: all the custom block related discussions should happen in the follow-up. Right now there is no way to add custom blocks on the Block layout page but instead on the custom block library page.
Comment #115
googletorp CreditAttribution: googletorp as a volunteer commentedI'm generally happy with the code as well.
There are some small nitpicks that we could address, like
Conversion to new array syntax in related code.
Correctly use protected (or public) for class methods.
These changes are actually scope creep, not sure if this is a showstopper. The code is nice and clean like it is, but might be breaking a few policies.
Comment #116
MattA CreditAttribution: MattA commentedFollow-up issue for the custom block module: #2523154: Improve workflow for creating custom blocks
Comment #117
MattA CreditAttribution: MattA commentedSmall nit, update the label description to: "The element containing the block name." Even with the type-hint, the description makes it sound like a string and I'm getting very tired with the poor (to put it nicely) documentation in core (mostly class properties that just restate the variable name instead of actually explaining what it is or what it is used for).
Also, might want to rename that function too since it doesn't just "show" rows.. it toggles them.
@googletorp Oh, you have no idea how hard I had to hold myself back from setting visibility modifiers on those... sadly, I determined that it probably was issue creep. :(
Comment #118
MattA CreditAttribution: MattA commentedComment #119
legolasboFixed both nitpicks in #117 and a array notation i found because of #115-1.
In regard to #115-2, I don't think changing the other visibility modifiers should be part of this patch.
Comment #120
dawehnerThank you for adressing the ordering issue mentioned before!
I think the points in #115 are valid, but should not block the issue from being done.
Comment #121
xjmNice to see this RTBC!
I think we will need @webchick's signoff on this one for the product implications, so assigning to her.
Comment #122
MattA CreditAttribution: MattA commentedAssuming the testbot agrees, +1 RTBC.
Comment #123
googletorp CreditAttribution: googletorp as a volunteer commented+1 on RTBC
Comment #124
tim.plunkettI'd like a chance to review this as well, as the block.module maintainer.
Comment #125
Bojhan CreditAttribution: Bojhan as a volunteer commentedShould show the actual block type, not just "custom".
I know that we are pushing to kill the criticals off, but I'd like to leave this on needs review for a bit to make sure we are not missing any gaps.
Comment #126
MattA CreditAttribution: MattA commented@Bojhan The list generation is pretty much a copy/paste of the old code. Currently, core just says "Custom" too, so that should be a separate feature request.
Comment #127
andypostAlso #125 shows "Block layout" twice in breadcrumbs so needs to be fixed here
Comment #128
dawehnerI'm sorry but noone is stopped from looking at RTBC issues ...
Comment #129
MattA CreditAttribution: MattA commented@andypost So trivial... and actually from the a menu we haven't touched, but will update with fix in a few.
Comment #130
legolasbo#127 fixed locally, waiting for tests to pass.
Comment #131
MattA CreditAttribution: MattA commented@legolasbo Oh hmm... well here's what I had. First part is the fix for #127, but I also added some more context to the page by adding the theme name to the page title since we probably shouldn't rely solely on the breadcrumb for that.
Comment #132
legolasbo@MattA,
I was just about to upload my patch, but I've merged in your changes aswel. Unfortunately I don't have enough time remaining to wait for all tests to finish (the tests pass so far though).
Comment #133
legolasboWe do need tests for the Dynamic title.
Comment #134
xjmTagging based on @tim.plunkett's comment in #124 and my comment above. (Reference: #2457875: [policy] Evolving and documenting Drupal core's structure, responsibilities, and decision-making) Once we get these signoffs, let's list them in the summary. We can wait to assign to @webchick until it's RTBC again (after @tim.plunkett's review, fixing the bug @andypost found, etc.).
@Bojhan, did you want the opportunity to review it further as well yourself? Tagging for that too. When you think it's acceptable, please untag and mention it in the summary. :) I am sure @webchick will be glad to hold off on committing it for a few days if it's just for more general feedback.
And finally, NW for the added test coverage.
Comment #136
MattA CreditAttribution: MattA commentedJust going to see how tests do with a clean version of the patch from #131 (also has a title test).
Comment #138
legolasbo@MattA,
It seems we've taken a different approach to solving the same problem. I like your solution, but my approach matches the pattern already used by the block(_content) module. Which is why I think my pattern is a better fit. But since I don't have any time to work on this during the weekend I'll leave it up to you.
Comment #139
andypostmore nits
Looks better to use controller's defined title callback to make like "Blocks for @theme"
trailing white-space
Comment #140
MattA CreditAttribution: MattA commented@legolasbo Umm, might be too early for me, but I don't follow. What pattern is that?
@andypost Same with #1 here. I don't know what "controller's defined title callback" is referring to, but I think the breadcrumb title should match the tab title, so going to keep it as is for now. Fixed the whitespace though.
Comment #141
MattA CreditAttribution: MattA commentedComment #142
legolasbo@MattA,
In
core/modules/block/block.routing.yml
two routes are defined for the block layout page.The first is used for the default theme, the second for all other themes. Your patch does not provide this 'pattern' while mine does. Besides that i think the path used by the current patch (
admin/structure/block/list/{theme}/add
) is not really descriptive of what should happen there. whileadmin/structure/block/placement(/{theme})
describes the main action at the resulting page, which is placing a block.For the same reason I also think the path
admin/structure/block/<strong>list</strong>/{theme}/add
should be changed toadmin/structure/block/<strong>layout</strong>/{theme}/add
. But that's for another issue.Comment #143
lauriiiI'm gonna try to merge two patches we have here so that we have one patch everyone can work on! I'll come back with a patch soon.
Comment #144
lauriiiI won't write a patch here because of the differences were smaller than I thought. Just as an future reference it is very confusing if there is patches going in their own trees, especially if its not clearly documented what are the differences between them.
I guess what this is mostly about is
Patch #132 has path structure of:
Patch #140 has path structure of:
In #140 user is always redirected to a URL containing the theme. Also the BlockListControllers layout method has the layout as a required parameter in #140.
I personally think that the structure is better on #132, but it might be worth changing
/admin/structure/block/list/{theme}
into/admin/structure/block/{theme}
Comment #145
MattA CreditAttribution: MattA commented@legolasbo Ah I see. Well both of those paths were already there, and the new page needed a path with '{theme}' defined, so it was logical to create the new page there (not to mention the only option).
That said, we are getting into issue creep. Again. The goal here is to clean up the UI layout. We can save making route paths prettier for later.
Comment #146
lauriiiI'm okay with fixing that later.
Comment #147
MattA CreditAttribution: MattA commentedMinor note: Will need a re-roll, depending on which goes in first, due to #2514998: Reduce fragility in the monolithic BlockListBuilder.
Comment #148
legolasbo@Lauriii, #132 already has MattA's page title 'feature'. All it needs is a few tests fixed because they still refer to the paths introduced by MattA. Since those tests were altered before to make the new page work, they shouldn't be hard to find.
@MattA, I disagree on improving the paths later. We've introduced this new page in this patch and should make sure we get it right the first time. It's a minor detail and we're still waiting on the UI changes to be approved anyway. So let's just fix the tests in my patch #132 and we're good to go with proper paths.
Comment #149
bill richardson CreditAttribution: bill richardson as a volunteer commentedI find the proposed changes ( and after testing patch ) makes the block placement very much harder and less usable than before.
I can see the benefits of rearrangement of custom block button , but has anyone tested with just moving the sidebar to the left and leaving the block sidebar as is ( i personally love this feature but do agree that the + sign beside the block is confusing as it gives the impression that you can drag the block into a region ).
Comment #150
MattA CreditAttribution: MattA commented@bill richardson Yes. All the way back in #16. See #17 for why not.
Also, just for clarity, by "block placement" do you mean moving an existing block around (although that hasn't really changed) or adding a new block? Assuming it's the latter, we have a couple follow-up issues to help, such as #2513528: Add a link to add a block in empty regions on the Block layout page. and #2513520: Add a contextual link to add a block to a region. So once the follow-ups go in (and hopefully the intermediate pages get ajax-ified like they were in #55) the old sidebar functionality should be back.
Comment #151
bill richardson CreditAttribution: bill richardson as a volunteer commentedIf we have to proceed this way , at least implement mockup1 , where region was displayed in a column so that a block could easily be moved to another region via the dropdown .
Comment #152
MattA CreditAttribution: MattA commentedIt's still there, click 'show row weights'. Currently the sortable table is focusing on its job, which most people apparently prefer using. So if you want to use the select list (and get rid of that pesky drag icon) you'll have to click that exactly once, since your browser will remember the setting.
Comment #153
dawehner@bill richardson @MattA @legolasbo
Whatever happens here keep in mind that a) we really want to fix the critical part of the issue and b) we can iterate easily and refine/improve it later
Comment #154
legolasbo@dawehner,
Both patches (#132 and #140) are identical except for the fact that #140 uses sub-optimal paths and #132 stil has a few failing tests (because of the changed paths). I'm fine with #140 going in as is and fixing the paths in a follow-up issue. However if that doesn't happen before Monday morning I'd prefer to fix the tests on #132. In any case I'm not planning to do any further UI changes. I think the current patch(es) solve the issue at hand and any other UI changes should be and will be addressed in the follow-up issues which are already created.
Monday i've got all day 10:00 - 17:00 (GMT+1) to take this home. The tests (for #132) should be fixed within an hour, which should be the last bit to finalise the patch. Any new feedback from a UX review or review by subsystem maintainer or product manager can be fixed then aswel. I'll be on IRC in the mentioned timeframe if any discussion is required.
Comment #155
MattA CreditAttribution: MattA commentedI still think it should be a separate issue, but I was also thinking, what if instead of adding more routes, we just rename the original. So instead of:
Note: Really, route 3 shouldn't even exist either, because you should just have route 4 with {theme} as an optional parameter and dynamically change it in a
RouteSubscriber
.We just have something like:
The path for the add action would then become:
/admin/structure/block/layout/{theme}/add
.This allows us to not add more routes, not add additional code required to get the default theme in the controller and not have to subsequently create a new test. With the added benefit of the current theme tabs being under
block/layout/{theme}
which makes more sense than leavingblock/list/{theme}
in.Comment #156
lauriiiJust reminding here that which paths block layout is providing shouldn't be part of a discussion in the critical issue. How does it make the UX of the block layout better? It is important to have sane path structure but it can fixed in a normal/major follow-up.
Comment #157
bill richardson CreditAttribution: bill richardson as a volunteer commentedThe page title missing when on custom blocks tab with sub menu block selected.
Comment #158
MattA CreditAttribution: MattA commented@bill richardson That's actually a problem with the Views module from what I can tell. Same thing happens with the front page view on my system. "Welcome to Drupal 8" disappears. If you disable the module you can see that it isn't a bug related to this.
Comment #159
MattA CreditAttribution: MattA commented...was added/fixed way back in #140.
Comment #160
webchickWow, this issue has been busy! :)
I'm out of the loop on activity since Thursday (will try and catch up tonight), but I can only reiterate the pleas from various folks to please keep this patch the barest minimum required changes in order to fix the critical. We have the rest of the entire 8.x cycle to make additional improvements. But by all means, since you're inspecting the code here anyway, please do spin off sub-issues for any weirdness you find (such as paths or whatever).
Comment #161
bill richardson CreditAttribution: bill richardson as a volunteer commentedMattA - yes the page title error is in beta 12 --- nothing to do with patch --- sorry for the noise.
Comment #162
MattA CreditAttribution: MattA commentedYeah, no changes have gone into the patch, the only thing happening has been discussion. Went ahead and created #2526766: Improve naming of block paths for @legolasbo though. Any relevant comments can go there now.
Comment #163
MattA CreditAttribution: MattA commentedI was going through the block module's files for a different issue and it seems like we forgot to remove the block-list.html.twig file that was used for the two-column layout (and it's hook_theme entry).
Comment #164
legolasbo@MattA,
You are right. I actually removed those in #31, but you forgot them in your fresh start. I'll remove them and will also check if we've got tests for all relevant changes.
Comment #165
MattA CreditAttribution: MattA commentedAnyways, also renamed BlockLayoutAddLocalAction to BlockLayoutLocalAction since it's technically reusable with any action on the block layout page (although we've probably learned our lesson about that...) and removed an unused trait.
Comment #166
MattA CreditAttribution: MattA commentedComment #167
legolasbotestCandidateBlockList()
used to also test the block placement URL but that test was removed. Reintroduced that check.Comment #168
legolasboWhile reviewing #167 to find additional things which should be tested, I encountered a discrepancy with regards to the new block placement page.
On
/admin/structure/block/list/bartik/add
(default theme) there is a breadcrumb trail. If i were to click the last of it's items ('bartik')..
Expected result:
I would expect to navigate to
/admin/structure/block
since bartik is the default theme..
Actual result:
However this link leads to
/admin/structure/block/list/bartik
which has broken local tasks because the 'bartik' task is not selected..
Solution:
I solved this problem by implementing the path change I suggested in #132 which does 3 things for us.
Besides this I've also changed any tests which were using hardcoded paths to use paths generated from the route, which should make a future change of the paths (if needed) easier to implement. To keep the scope as small as possible I've only touched things which we were already changing to begin with.
Comment #170
legolasboOops, I missed a use statement.
Comment #171
legolasboI’ve walked trough all comments since the last RTBC status in #120 to find any actionable tasks and list their status.
#125 - Bojhan - Category should show the custom block type, not just Custom.Created #2527522: Show block type in stead of "Custom" on block placement page#127 - andypost - “Block layout” is shown twice in the breadcrumbsFIXED#133 - legolasbo - We need tests for the dynamic titleNo longer relevant, dynamic title removed in latest patch.#147 - MattA - Will need a reroll if #2514998: Reduce fragility in the monolithic BlockListBuilder lands first.
So assuming testbot agrees, I think we're there. Only needs another proper review.
Comment #172
dawehnerYes I agree the active link-ness is a bit sad ... but I think this does not need to block the issue at all. It is just a clear bug in our local task handling.
I gave this patch another manual try, it fixes certainly the critical issue. The code is looking fine for me as well, so I'll RTBC it, even people disagreed
with that, due to its tags.
Ah we now have #markup
Just for the future ... try to simply avoid those changes, they add noise and make the patch a little bit harder to review.
Nitpick: Add a new line between, can be fixed on commit.
Comment #173
plachSubsystem maintainers and the UX team can still mark this as needing work, if that's the case, I assume @webchick won't commit this until they are happy with it (and she is :)
Comment #174
webchickActually, I'd love for Tim to take a look at this first. He's the Block module maintainer, and also the original coder of this UI.
But yes, would love to take a look at this before it goes in. This week isn't a travel week for me, and this is a critical issue, so I will definitely make time for it.
Comment #175
tim.plunkettLeaving the "Needs subsystem maintainer review" tag on.
Gotta love out of scope changes! :( None of these lines needed to change at all, only the middle $output line had a change.
Not clear why we set the page title to one thing here, and then override it later. Is it just breadcrumbs? Why not fix that in a breadcrumb builder instead, at the correct level?
More completely unnecessary changes
This is supposed to be handled by the block manager, it should not be done here.
When moving this out of BlockListBuilder, switching the array syntax really didn't help keep it as reviewable :(
Shouldn't this new route (and all the rest) omit /classy from the route, just like the previous path did?
More unnecessary patch bloat.
This change makes me VERY nervous. This is a security test, why is it being changed?!
...
As before: Compare these two changes. The second still passes the theme, the first adds in 'classy' for no apparent reason. Use the other theme-less route or we'll lose test coverage.
Comment #176
tim.plunkettAlso, why is the modal removed from "Add block"?
Comment #177
dawehner@tim
I hope you don't try to be defensive on this issue.
Well, because its now rendered differently, just try it out.
Comment #178
Bojhan CreditAttribution: Bojhan as a volunteer commented@tim.plunkett For this see #101. It basically comes down to the following. The way we use modals in Drupal (and should) is to show a modal dialog when you want to make a "quick action" and then return to the listing.
Currently we only apply this pattern in Views and the blocks UI. Now that we are moving it to a separate page, having a modal on the listing you don't return to is non-sensical. It provides no valuable context or point you typically return back to. As when clicking save you do not return to the "add block" listing -> instead you go to the blocks layout page (which in this usecase is the preferred flow).
I'd also like to add that for consistency with core UI patterns, it makes sense to remove the modal from this place. Sadly its very poorly implemented as a standard interaction across core. You currently only encounter it in the views UI and CKeditor to some extent. The blocks UI implementation is kind of a one-off, since we don't provide this pattern on any other listing.
Comment #179
tim.plunkettThanks @Bojhan and @dawehner, makes sense.
#175 parts 1-7, 9-10 still remain.
Comment #180
dawehnerQuickly addressing this together with some small other stuff.
Thank you tim!
Comment #181
dawehnerI hope I managed to address all the points from tim.
Comment #182
googletorp CreditAttribution: googletorp as a volunteer commentedAdding missing documentation to toggleBlockEntry in block.admin.js to make it pass ESLint. Since we made changes in the function that would require the docs to be updated anyways (change name of variable) this should be part of the patch. Not sure why it got removed, since it was added earlier.
Comment #183
tim.plunkettThe JS changes weren't reverted at all, but oh well.
Revert please
Tested this like this:
and it is broken. Needs tests! :(
These are still the wrong routes (should not hardcode classy)
Comment #186
tim.plunkettRemoving BlockLayoutLocalAction, we don't need it.
Fixing #183.1 and #183.3, point 2 still needs work.
Comment #187
tim.plunkettWhoever rerolls next, please remove these :)
Comment #188
tim.plunkettI'd like to put in my 2 cents:
I do not agree with this approach, I think it is heavy-handed and brings us further away from an ideal UI.
Moving it to the left and tweaking some wording would be enough to make it no longer critical, and allow us to work on a better interaction pattern.
That said, I won't hold this up, and barring any drastic changes other than adding test coverage and fixing the XSS, I think this is implemented correctly.
Comment #189
jibran+1 to the current direction. IS needs an update of screenshots.
Comment #190
MattA CreditAttribution: MattA commented1.
admin/structure/block/layout
should beadmin/structure/block/add
It was
admin/structure/block/list
that should have been layout, but yea, patch minimalism and everything.2.
Seems... less than ideal.
3. Regarding the JS, while it may look similar, it was actually rebuilt from scratch. Quite frankly I think 5 seconds of extra review time is worth the better variable and function names there too. :P
Comment #191
webchickWell spotted, but note that smaller issues like that can easily be follow-ups once the critical problem is fixed.
Comment #192
dawehnerI think the product manager should make a deal with the subsystem maintainer to ensure that we don't block each other.
Comment #194
tim.plunkett1) Those paths are pretty wack, now that I look at them again. Going to see what I can do about that.
2) Just to be clear, it looked like that before my changes... Might be follow-up worthy.
3) Fair enough
Comment #195
tim.plunkettChanged /admin/structure/block/layout to /admin/structure/block/library. It's not the layout, /admin/structure/block is. Renamed BlockLayoutListController::layout() to BlockLibraryController::listBlocks() accordingly.
Also added the tests and fix for the XSS issue.
Comment #197
dawehner+1 to make sense!!! Maybe the route names could go with plugin_library, to distinct maybe from the custom block one?
No, yes?
We need to go deeper!
Comment #198
tim.plunkett1) Custom Block doesn't use the word library any more as of this patch
2) Whoops!
3) Heh.
Can someone update the screenshots in the IS to match reality?
Comment #199
dawehnerAh, good point.
+1 for RTBC-ness of that issue, but I have been too positive in the past here.
Comment #200
jibranIt needs a change notice as well. NW for every need tag.
Comment #201
larowlanFew observations, mostly nits but one where I think we're losing some coverage
nit: can we use " here instead of ' and \'. I recall using \' as bad form for translators
should this be Test XSS in category given the method name?
Shouldn't we also be asserting that
<script>
doesn't exist?Comment #202
larowlanJust did some manual testing.
As maintainer of block content module, I think this issue is a step backwards.
Drupal 8 comes with a killer new feature - fieldable blocks and block types.
But you'd struggle to find it.
How do you add a new custom block now?
Not here (and note no help text pointing me to the tab):
or here (again no help text):
I think we're burying a killer feature.
Sorry to be negative, but kind of makes me a sad panda to see stuff you worked hard on buried away.
So I think we're missing two major things here:
a) Help text on the block layout page indicating how to add a custom block
b) Help text on the 'add block to layout' page.
Comment #203
Fabianx CreditAttribution: Fabianx as a volunteer commentedI share #201.3: This needs a comment.
Just saying it changed, is not enough. As this stands now, from the outside it seems like a security regression.
Comment #204
xjmAlso needs a reroll following #2514998: Reduce fragility in the monolithic BlockListBuilder -- if the reroll turns out to be onerous we can revert the other issue. I pinged @tim.plunkett to have a look.
Comment #205
MattA CreditAttribution: MattA commented@larowlan We were well aware of how custom blocks would be nearly impossible to find when we removed the action link, which is why we created #2523154: Improve workflow for creating custom blocks a while back. So far, we're thinking about adding it to the 'admin/content' path, which would be a major step up from where it was, especially since pretty much every normal person defines blocks as content anyways! :)
Comment #206
tim.plunkettI'll handle the reroll. I'll be discussing this issue with Bojhan today, I'll include talking about custom blocks.
Comment #207
tim.plunkettThis in no way needs a CR.
Straight reroll, no changes.
The other issue really isolated the changes this has to make in BlockListBuilder, yay.
Comment #208
googletorp CreditAttribution: googletorp as a volunteer commentedThis JavaScript doc block needs to be updated, as the variable name changed. Might as well add the missing documentation since we are touching this.
I've added this myself a few times, not sure who keeps removing it, but the docs does need to update. #182 has interdiff with the missing stuff.
Comment #209
jibranWe have had change records for these kind of ui overhauling issues see Adding/re-using existing field in Field UI is now a separate task, Menu link UI now an autocomplete, with optional manual path entry and For Configuration Entities Delete is a button/link not a tab.
Comment #210
Bojhan CreditAttribution: Bojhan as a volunteer commentedFollowing tim.plunkett and larowlan their concerns we decided to have a call. Webchick, tim.plunkett and Bojhan have spent an hour going over the various concerns and thinking of solutions. This is a write up of that call collaboratively written.
Previous testing
In the testing we did (to add some clarification we saw that users):
We asked people to place a list of recent registered event attendees in the sidebar. (going for "Who's new" block)
Current solution and its challenges
The solution that we arrived at in #17 was to remove the major confusion of add custom blocks being the primary action and replacing it with a block placement as the primary action. With that, aligning it better with core design patterns.
However, in the feedback we identified the following three issues with this approach:
Solution
We need to shift our approach a little bit to come closer to the core of this problem. The current patch does not solve the issue of people gravitating towards the regions themselves. Whatever sidebar position, or add button we place - people will continue to gravitate towards the idea that regions are the primary thing being shown on the block placement page.
With that in mind, we feel a better solution would be to bring block placement action more in context of the regions on the page.
We propose to do the following:
This solution should handle all the concerns we identified, while keeping a sane approach to getting this issue fixed.
Updating the issue summary with this new proposed resolution.
Comment #211
tim.plunkettLeaving this assigned to myself, will work on a first patch maybe tonight, or definitely first thing tomorrow.
Comment #212
larowlanThanks @Bojhan
a) this looks great
b) it will provide a nice sample-case of multistep modals in core.
Comment #213
webchickUpdating the issue summary's proposed resolution based on the new proposal.
Also noting that we're now officially rolling in #2513528: Add a link to add a block in empty regions on the Block layout page. as part of this solution now, to remind myself to credit those folks on commit as well.
Comment #214
webchickAdding a note that we should not call the buttons next to all the blocks in the library "Add block" but "Place block," so they don't get confused with the "Add custom block" button above.
Comment #215
Wim Leers#210 sounds great.
@Bojhan++
@tim.plunkett++
@webchick++
Comment #216
davidhernandezQuestion - with this solution, how will one create a custom block without adding it to a region? Also, will the disabled blocks still be available down below on the main page?
Comment #217
MattA CreditAttribution: MattA commented@Bojhan Thanks for the roll-up. So if I understand it correctly, we're basically sticking with the plan we had to begin with? Notwithstanding the slight modification for the action button which could have been part of #2513580. Also, to be clear, is part 5 still a separate issue? Seems like some of the stuff brought up in #2523154: Improve workflow for creating custom blocks would be out-of-scope if not.
@davidhernandez From what I can gather, the custom block tab should still be there in the main page and disabled blocks have their own issue.
Comment #218
tim.plunkett#216, the same way you do now, via the local task on admin/structure/blocks (the local action in HEAD will take you into the place block workflow)
Disabled region is being removed in #2513534: Remove the 'disabled' region from Block UI
#217, not sure what the original plan was, but it's probably close.
We're just moving the custom-block-into-place-block-workflow button from admin/structure/block into the modal, it doesn't step on the other issue that much.
Comment #219
Gábor Hojtsy#210 makes a ton of sense to me too. I went to try the latest patch and it was indeed far from as intuitive as the plan in #210 sounds like.
Comment #220
davidhernandezJust to be clear, I'm specifically talking about creating a custom block without placing it in a specific region. Starting disabled. Right now, the "Add custom block" button on admin/structure/block and admin/structure/block/block-content ("Custom block library") both use the same form, block/add. It reads like that action would become the modal talked about in #210? Just making sure we aren't losing the generic functionality. Wanting to add a custom block without choosing a region is not an uncommon use.
Comment #221
tim.plunkettWhen going via /admin/structure/block/block-content, the path for adding the block becomes /block/add?destination=/admin/structure/block/block-content, which prevents it from going into the "Place block" flow. That will not change.
Comment #222
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
re:
I think something like this will likely be the common use in D8 with several competing contrib solutions arising. Here is one already that's interesting (despite the poorly named project) https://www.drupal.org/project/edit_ui
Comment #223
tim.plunkettHere's a first pass.
The only visual problem I know for sure, is that modals don't include other regions, so the local action (Add custom block) is not included :(
Comment #225
dawehnerIt is great that we have now more consensus.
Comment #226
tim.plunkettMissed a spot in the tests.
Comment #227
dawehnerIt looks like views, so it can't be bad ;)
Comment #228
googletorp CreditAttribution: googletorp as a volunteer commentedI've looked at the code, and all look good to me.
I've also done some manuel testing and all seems to work very nice.
All in all I think this is good.
One thing I noted, is that there is a lot of place block buttons, which can me it hard to figure out which one to click at glance, when there aren't so many blocks in the region. I'm not sure if this is an issue or not, I'll leave this up to the usability review.
Does the issue still need summary update?
Comment #229
tim.plunkettOkay, adding a workaround for that.
Nope, no more IS update needed.
Yes, I agree it looks crowded when there aren't many blocks, but I don't know what to do about that.
Comment #230
dawehnerCan't we just call out to the block plugin directly? Curious whether this would be a good idea
Comment #231
tim.plunkettIt doesn't even belong to this module, it's from block_content, so I don't think we should hardcode it.
Comment #232
larowlanThis looks great - only thing issue I can see is this:
We'll need more context here for non-sighted users - suggest
Place block <span class=visually-hidden>in the %region region</span>
pity the action links ul is part of page.html.twig instead of a theme wrapper.
Manual review next
Comment #233
larowlanSo the workflow for adding a custom block is now:
It feels a bit clunky, but is not really any different to what we have in HEAD so I think exploring remaining in the modal is OK to make a follow-up.
Comment #234
tim.plunkettGood call on #232. The code change is a little ugly, but only until #2513534: Remove the 'disabled' region from Block UI goes in.
#233 isn't a show-stopper to me, we already have #2523154: Improve workflow for creating custom blocks to handle that.
Comment #236
andypostTest patch:
1) Modal should not have operations - they are links to next page to configure block.
2) Place block is a second line under region title
3) revert #234 to add title hint
Suppose title should be enough for
accessibility
button-small class is not used and defined
Comment #238
tim.plunkettdoesnt follow the mock, will revert tomorrow.
If someone wants to fix the failures in #234, feel free
Comment #239
webchickYeah, links IMO don't work here. Links indicate to users that you're navigating off somewhere else (and in many places this causes you to lose your work, and thus can make them somewhat scary to use). Operation buttons OTOH make it clear that this is an action being taken. We could explore ways to make the page less visually overwhelming (e.g. change the button label to just "Place"), but let's do that in a non-critical follow-up.
Comment #240
dawehnerLooking at the test failures.
Comment #241
tim.plunkett#236.2, button--small is defined in core/themes/seven/css/components/buttons.css line 130, and is used by seven_preprocess_menu_local_action() which we're mimicking here.
Thanks @dawehner, I can't deal with xpath tonight...
Comment #242
dawehnerXpath can't deal with me neither, take that!
Comment #248
andypost@webchick so what about to use links in dialog? this is exactly navigation purpose (navigate to block settings form) .. for me the same applies to "Place block" link in list - navigate to list of available blocks.
No data is could be lost because it's navigation
Actually I was trying to use contextual module links when hovering over region name but it looks that "shortcut module link style" (start near page title) looks better and more self-descriptive
does not make sense... we need to check link as a whole
An the link name now incredible long
Comment #249
Manjit.Singh@tim.plunkett @andypost when i am placing blocks to another region then it seems like it's background color is same as error. Please check screenshot and screencast i have attached.
Comment #250
tim.plunkett#248 I'm going to stick with the usability expert. If you want it changed from a button to a link, please open a normal follow-up.
"An the link name now incredible long"
That's what .visually-hidden is for, you'll only see "Place block", the rest is for screen readers.
#249, that's an existing condition, please open another issue.
I think I have a reasonable compromise fix, coming shortly.
Comment #251
tkoleary CreditAttribution: tkoleary at Acquia commented@tim.plunkett, @webchick
I think that since this issue arose out of the Minnesota usability test, in order to mark it RTBC the patch must pass follow-up usability testing.
Thoughts?
Comment #252
tim.plunkettInterdiff is from #234
This just adds a clickLinkPartialName() method
Comment #253
dawehner+1 for a dedicated method!
Comment #254
dawehnerWell, I think given that we have a similar pattern in views, users can use it already. People never had hard problems with it.
On top of that, we want to iterate for the future, so halting back now, is just a wrong thing to do.
Comment #255
tim.plunkettIf we have @Bojhan sign off on this, I think that's enough. +1 for RTBC.
Comment #256
tim.plunkett@Manjit.Singh #2396937: "Last placed block" color confusing in Block layout page is slated to address the confusing color
Comment #257
Bojhan CreditAttribution: Bojhan as a volunteer commentedA small issue, the "search box" on the blocks listing should be moved to the right (where we normally have the show weights link) to avoid any confusion about that two. We learned from testing that closing these elements close to each other, might have people perceive they need to enter something in it and then click "add".
The rest looks fine.
Do we have all the appropriate followups in place?
Comment #258
webchick@tkoleary This is a critical, and putting usability testing together is not easy, so I would -1 requiring it prior to commit. It's also much easier to usability test if the instructions are "download the latest Drupal 8 beta" and not "Clone Drupal 8 from Git, apply this patch..."
However, if someone would like to usability test it after commit, that would be very welcome. I'm pretty confident we will not hit another critical usability regression with this approach, but we could definitely find things that could be further cleaned up.
Comment #259
tim.plunkett@Bojhan @LewisNyman This "works", not sure what more you'd want. I know we shouldn't use IDs, but we need to be careful to only target the part within the modal.
Comment #260
Bojhan CreditAttribution: Bojhan as a volunteer commentedThis looks good to me :)
Comment #261
dawehnerI think
#drupal-modal
is fine, given that there will be just one modal on the page.Comment #262
tim.plunkettThis CSS doesn't work in FF, clear:left on the table works, but I'm just going to leave this for lewis.
Sorry @dawehner
Comment #263
webchickFunctionality review from a "product manager" POV:
Everything works as expected. I can click the "Place block" button next to any region and its region selector gets pre-populated correctly. The filter in the block listing is working great. If you want to add a custom block without putting it in a region, you can do that from the "Custom block library" tab. I tried various ways to break it and could not. AWESOME work!
Here's my list of specific commit-blockers/follow-ups:
Comment #264
tim.plunkett#263
2) If we don't want block search on the right, then #252 could be committed... #257 had the reasoning
3) It has a label but it is visually-hidden, so that's not a bug
4) Follow-up needed
Comment #265
webchickOr, in lieu of follow-up 1a, I would rather we go back to the patch in #252 which had the search/add buttons together, and open a follow-up (one probably already exists?) about the valid issues that #257 is pointing out came through in testing. We do need indeed to look at that, but IMO we need to do so systematically, not one-off on this page.
Comment #266
webchickOk, it looks like attempting to move the search bar to the right is what caused the Firefox issue to begin with.
Therefore, I'm going to RTBC #252, and aim to commit this this evening in order to allow Bojhan a few hours to respond.
Comment #267
webchickSaving credit.
Comment #268
Bojhan CreditAttribution: Bojhan as a volunteer commentedAlright, lets move this in. Webchick informed that this was a pre-existing condition (found on modules page for example). Sadly we are exposing that problem here again, while its annoying - we should not hold up commit on a relatively small issue that can be solved in followup.
Can we please make sure we assign appropriate priority to these small but still usability problem inducing issues?
Comment #270
webchickOk, then... we are a-go!! :D
Committed and pushed to 8.0.x. YEAH!! Was so great to see this come together so quickly! Thanks to everyone for their help!!
Comment #271
webchickAlso tagging for follow-ups. I think we need at least two: one about the search problems mentioned in #257 (may already exist and be tagged "UMN 2015") and one for some minor cosmetic changes we might want to make for button labeling, etc.
Comment #272
almaudoh CreditAttribution: almaudoh commented+1000. A lot has gone on in this issue since I last looked. Great job folks!!
For me, the most important thing is like @larowlan said in #212:
I'm going to be one of the first beneficiaries of multistep modals' sample in core.
Comment #273
MattA CreditAttribution: MattA commentedSeems we lost all the help text updates too, "click the block title under Place blocks" references the old sidebar (although oddly enough it would've made sense with the UI in #236).
Comment #274
nod_Comment #275
legolasboAdded #2532054: Move the block layout page to Appearance as suggested in #210.
Comment #276
tkoleary CreditAttribution: tkoleary at Acquia commented@webchick
Sure, that makes sense. Melissa Leach and Julie Fischer here in Boston have said they'd be willing to test it.