backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

Implement a drag & drop model for building layouts #86

Closed Bojhan closed 10 years ago

Bojhan commented 11 years ago

A big part of Drupal's UX flaws is its inability to provide a great experience for some of the core site building concepts. I have raised this concern many times, and we even outlined it as a strategy for Drupal 8.

Given that Backdrop seems to focus on this sitebuilder type of audience, I'd love for this to happen. Clearly its a big effort, but in terms of UX it will set it apart. From my point of view, improving UX by removing a bunch of stuff - does very little if you leave the core concepts intact, that is what most people will care about.

The actual model could be rather simple, you need to decide between either real live or abstracted view of "a display". This display can then be configured, by pulling items (blocks, fields, menu's, etc.) on top of it. The tricky part is making this a cohesive experience across data models.

I wouldn't mind helping with the interaction design and research, but the biggest problem is and has been resources.

saltednut commented 11 years ago

I am interested in looking at this too. Are we looking at porting any existing contrib or creating a new system when its decided how plugins should work in backdrop? I think it might be a mistake to fork panels due to complexity. It would be nice to have a Panels IPE-like experience without the extra baggage provided by the full panels system. Distributions like Panopoly have shown us that setting something like this up in Drupal 7 required a lot of different modules. In addition, it involved greatly changing the default UX provided by ctools (re: panopoly_magic, and *_widgets)

quicksketch commented 11 years ago

Hi @Bojhan, thanks for opening this issue! I've said it elsewhere already, but the number 1 thing I'd like to see in Backdrop 1.0 that's not in D8 is a layout tool. I saw an earlier post you made ("Drupal component library" or something like that? I can't find it at the moment), that lined up pretty close with what I was thinking of also.

I think for the first pass we should not attempt to do an in-place editor or front-end manipulation. It has too many challenges around describing which pages you're editing: whether it's a single page or multiple. Not to mention rendering and moving around content with actual things inside of them is both heavy (web browsers can have a hard time moving huge chunks of the page) and difficult to implement.

I think we can learn a lot and borrow heavily from Panels, but for an out-of-box tool Panels demands too much knowledge to operate.

The basics of what I'm thinking of consist of the following:

The concept of "context" is also important to address, but honestly even D8 didn't figure this out and currently blocks of no concept of context whatsoever. Starting with building a foundation for stand-alone layouts that can replace the terrible one-layout-for-everything would be an excellent start.

dalejung commented 11 years ago

So this is from my experience with D6 and Panels, it's a few years old, I hope it applies.

I think that the drag-and-drop layout should be decoupled from panels or whatever the new system is called. In practice, people rarely change the layout and bigger editorial teams will have set templates.

The reason we used panels was for semantic groupings of plugins. This was useful for tiered caching, as many panels would invalidate as infrequently as their components. In most cases we were able to get a cache hit on the entire panel and not just individual blocks.

Another big reason was the ability of panels to pass in args to their plugins. Having the ability to decouple each plugin from what was going on outside it made re-use incredibly simple. Sometimes the nid came from the url, sometimes from a node references, the plugin didn't have to care.

When it came to the actual layout, the panels rendered each plugin and then threw them to a tpl. In this way, we didn't necessarily care about the ordering of the plugins. This was especially true for more complicated layouts that didn't fit any type of grid. The Panel was just there to define an api for plugins and group them into tpls. Obviously we did modifications to remove all the automatic html wrappers and make the tpl override names easy.

This was by far the most robust system I could come up with. Themers were happy because the DOM made sense to them and they could accomodate any design.

My worry is that a pre-occupation with layouts would make it harder to decouple all the benefits listed above from the drag-and-drop gui.

Also, this my be reflective of my experience with magazine publishers, but we could never get the "whole page is a panel" to make sense. If you suppose that every page is really 3 regions (left, content, right) and you have 3 variations of those regions, then that requires you build 27 panels for each permutation. What we did is have each variation be a special panel that was tied to a region called a panel_template. These were outputted via a panel_block that selected the proper variant via a cascade of default->context->node. I suppose one could put a panel_block into a page wide panel, but I assume people are talking about a flatter hierarchy than that.

dalejung commented 11 years ago

Oh, the other added benefit of plugins that define their arguments is that you can automate invalidating their caches. Generally we had still make a pass and make the invalidation rule more granular, but it's still better than timed/not cache.

Bojhan commented 11 years ago

@quicksketch Interesting, I think our original ideas do encompass much of this experience. Frankly I could hash out the key parts in under 15 minutes. But the challenge is not in the solution, but in the scope. I'd love to get the basics of a layout placement system with drag and drop before even thinking about page creation and selection filters.

The whole reason most of the SCOTCH work stranded, is because we were too ambitious. Given that we are dealing with far fewer resources here, I'd aim for getting the basic block drag and drop in regions, layouts that represent the actual groups, a nice way to select the right block - first. Then expand the scope to creating page, and finally mapping different layouts to one URL.

The whole problem with Panels et al, is that it focuses on doing it all - Backdrop should by its nature start with getting the key patterns right.

matt2000 commented 11 years ago

May I suggest:

Layouts:define regions available and their placement in the page Contexts: define what goes in regions, in what order Themes: define the markup/style of regions and things in them; also how the 'order' defined in context is interpreted.

jenlampton commented 11 years ago

I re-titled this issue "Implement a drag & drop model for building layouts" because blocks and fields should probably be two separate issues, but the bigger picture here is layouts.

duckzland commented 11 years ago

so far the best layouting system via GUI that I've encountered is WordPress Headway theme framework. I believe that kind of system can be integrated with Drupal theme system renderable arrays. Basically things like context, display suite, fields, blocks, regions is mashed up together in a single Layout array.

each page has its own layout array stored, the layout array can be reused for other similar pages, such as page.layout will serve all pages, page--blog.layout will serve page with blog node type etc..

the structure of the array can be something like : -- Area Header -- Region Logo -- Block Logo -- Region Navigation -- Block Menu 1 -- Areas (or wrapper) Content --- Regions Content --- Block 1 --- Field 1 displaying title --- Field 2 displaying content --- Region Sidebar --- block C --- Block D --- Field C --- Area Footer -- Region Footer left -- Region Footer Right

I've implemented such system in http://drupal.org/project/sigmaone but isn't expanding it further due to shifting focus to WP.

Hope this helps in creating something like headway for backdrops.

quicksketch commented 10 years ago

Hi guys, sorry this issue is starting to look like one brain-dump after another, and I'm about to add another one. :(

@jenlampton and I went through the ideas of the basic plan above and refined it to particular paths. Calculating how to deal with the options we're going to be having (i.e. contexts, arguments, etc.) It's not a fully baked plan but it's a start, and more specific than the general outline above.


List of Layouts page: admin/structure/layouts

Add new Layout form: admin/structure/layouts/add

Form includes options for:

Save the layout setup. On to the next page: the layout content.

Layout Edit page: admin/structure/layouts/manage/[layout_name]

When editing a layout, the "Settings" form for the layout containing the original options may be access via a tab: admin/structure/layouts/manage/[layout_name]/settings

quicksketch commented 10 years ago

@Bojhan pointed me at his video of a mockup at http://bojhan.nl/blocks-layout-prototype, which is remarkably close to what's being suggested here. The idea for setting up paths, arguments, and contexts on the first page is almost identical. Some of the further ideas (additional data sources) might be out of our initial scope, but overall it's a great visualization of approximately where we'd like to take this.

quicksketch commented 10 years ago

I'm starting to make headway on this issue. Although the overall amount of work is a little intimidating, I think by leveraging the existing code from Panels, this should be a surmountable task. Instead of a "ground-up" implementation, I'm working at this task from the perspective of porting the architecture of Panels, with the UI described above and in Bojhan's video. So essentially this becomes a porting and UI job instead of trying to reinvent the wheel. I'll post to my sandbox when I get something worth sharing.

quicksketch commented 10 years ago

My work on Layout module still isn't close to pull-request status, but I've been making steady progress for the last month. The in-progress branch is located at https://github.com/quicksketch/backdrop/tree/86/layout_module

Despite my intentions to mirror @Bojhan's mockups, we're ending up with something a lot closer to the Panels UI than I had anticipated. Because we're basically "porting" Panels' concepts to Backdrop, the UI ends up following suit in a lot of ways.

So far, the only things working are creation of new layout paths, creation of new menu items, the very basics of automatically and manually identifying contexts, and saving/loading/cloning/etc. Some screenshots:

Layout listing: layout-overview

layout-setting

There's still a ton of work to do, but at least this shows some progress.

quicksketch commented 10 years ago

I've still been pushing forward on layouts. Recently we got to a point where you can actually add and move blocks around on the page, which is a great milestone in the building out of this module. While the module is somewhat stable, I made a branch for anyone who wants to try it out at https://github.com/quicksketch/backdrop/tree/86/layout_module_20140716_demo.

I'll be putting together a video to demonstrate the current status of things shortly.

quicksketch commented 10 years ago

Here's the video of current status: http://youtu.be/pMKcjEetKAg

It's coming along nicely and actually has some usefulness now. The largest remaining tasks are:

And then there are lots of little things on top of that: form contexts, integrating properly with views, finishing porting the available plugins, styling things... still lots to do for sure, but it's good progress and feels real at this point.

quicksketch commented 10 years ago

It's been a while since an update, but this has been pushing forward steadily. All 3 items from my last update are now complete. Layouts can completely take over an existing page and use (or not) the existing content on the page.

Additionally, we've built out:

I've also removed all remaining references to page manager/ctools/panels, and trimmed down the scope of the first merge to just include replacing the functionality currently in core. We can port the remaining CTools plugins as individual PRs at a later time after we get in the base architecture.

Here's a screenshot of the current interface, now with header and footer regions:

layout-interface-sept

As always, the progress is open and located at https://github.com/quicksketch/backdrop/tree/86/layout_module

I'll be preparing a new video with our current status as soon as possible. The only remaining tasks at this point are:

Bojhan commented 10 years ago

Wow :) I will give it a spin

quicksketch commented 10 years ago

That would be excellent @Bojhan! I'd love to hear your thoughts. Ultimately this ended up architecturally very similar to Panels since we started by porting the functionality. UI-wise it is simplified, but doesn't quite follow the workflow your original videos outlined. I remember the biggest take-away from your video was how additional contexts were added was very different than Panels today. For the initial version of Layouts though, we haven't added the ability to support relationships or even additional contexts (besides those from a URL). The infrastructure is there however, so after we get the basics hammered out we can work on the UI to support additional contexts and relationships.

quicksketch commented 10 years ago

Here's the current status of our tests. Mostly functioning but a lot of block-related failures: https://travis-ci.org/quicksketch/backdrop/builds/35674324

klonos commented 10 years ago

This is so exciting!!! Can't wait for the video demo as well as the chance to try a backdrop build that includes the feature.

Thanx Nate for all the hard work.

quicksketch commented 10 years ago

Glad you are excited about it @klonos! I am too. This is going to be a ridiculously huge improvement over the current system.

The new video has been published to our YouTube channel.

You can watch the video directly at https://www.youtube.com/watch?v=xoD6gDZmE3o

klonos commented 10 years ago

Just watched the video. Looks amazing!!!

Can't wait to start building my first Backdrop CMS theme.

Are you planning to publish any guide for themers that come from the D6/D7 world?

Bojhan commented 10 years ago

First off, whooa! I can only imagine the amount of work it took to pull this thogheter. Although it does not have all of the UX ideas that we put forward, I think it does a great job in significantly reducing the complexity compared to something like panels.

While I would love to help figure out some of the interaction design on this, I'd like to know how useful that is at this stage. I want to avoid my Drupal-type contributions, were I mostly just spend time making UI's that never go anywhere.

Long term

Preview / In-line model

The biggest step any of these tools would make is to provide a way to preview settings and/or a model to do it inline with preview. Its often hard to see how settings affect the "real" thing - ideally we pull that closer in every way possible. This will be the true differentiator between an implementation driven UI and a user mental model driven UI.

Smarter context selection and browsing

As we are adding more and more contexts, conditions, blocks etc. the real challenge will be in exposing a useable browser. This would be a separate issue, but perhaps something another contributor could help tackling. We should draw some inspirations how other systems do this, not Panels.

Short term

A lot of my comments here you probably already considered. But these are improvements you can make without major recamps.

Listing

The listing of layouts is a little odd, we don't really support "nesting" in tables very nicely in core. I will need to do a few explorations to see how we can make this look nicer. The new styling we have for tables would make a world of difference.

The default profile should probably come with two different context ones for content types and users? Good defaults really help here.

Layout viewer

There are a number of obvious visual improvements that we could make here. Non of these are critical for release. But mostly open doors, some ideas:

Known available contexts

In order to get a "node-type" context available. You have to know it exists. What would be nice if we have a description, or "recommended" contexts that a user can pick from. That would help with that discovery part (even if it is just the ones you support now).

I really like the auto-recognition, that really helps people understand what is going on.

Selecting context per block

A big leap that you could take here is introducing more of a "Browser" function. Similar to what Views does when you are browsing through available filters or fields. Right now its a drill-down which is relatively nice already! But it would be good for exploring purposes to allow you to browse and search for specific contexts.

Thanks!

quicksketch commented 10 years ago

The biggest step any of these tools would make is to provide a way to preview settings and/or a model to do it inline with preview.

While it's possible to do an actual preview (I actually had it doing that at one point already), loading the actual content can make arranging blocks difficult, e.g. blocks with a lot of content become so big it's hard to see the layout or drag a block from one region to another. The visual layout definitely helps a lot of eliminates the need for a lot of terminology (e.g. no need to explain what a "region" or "weight" is), so it's a big step forward already. In practice, I'm not sure using the actual content would be as helpful as it sounds.

As we are adding more and more contexts, conditions, blocks etc. the real challenge will be in exposing a useable browser

Absolutely. The current block selection list is a placeholder until we can refine that particular solution. My first thought was to do a search box + categories list, akin to the Views UI, but that's for consistency rather than because it's an ideal UI. If we come up with a new approach, I'd like to see it applied to Views and Layouts if it makes sense in both places.

The listing of layouts is a little odd, we don't really support "nesting" in tables very nicely in core.

Totally agreed. This was my 3rd or 4th attempt at making that listing of layouts. The list right now still has way too many buttons and options within those buttons. What I was trying to avoid was the Panels-like interface where you had to drill down 2-3 times before you got to the drag and drop block page. As that's what people want to get to the most frequently, I wanted it at the top layer, even though there's another concept (menu items and reordering layouts at the same path) that the layout itself is underneath. One idea had been to make the list of layouts under a path always be collapsed, then you'd click on an "Expand" button to show layouts under that path. But I think many (or most) paths will only have a single layout within them, with the exception of the node/% path, which almost certainly will have different layouts per node type.

Removing the buttons for adding and configuring, with nice +'s and cog wheels. That would make the UI easier to scan.

Agreed, this isn't already the case because it requires design direction. Although it's unlikely that people will be doing a page layout on an iPad (or is it?), we need to make sure that the icons are finger-friendly, so the existing tiny icons from Views are not going to be suitable.

Fixing the alignment. of blocks

Not sure what you mean about this one, what about the alignment is off?

Having conditions as a link that opens a modal, rather than a fieldset.

You can already see the list of conditions by editing the block, which opens in a modal. The idea was to help users when they have several blocks with different conditions figure out which one is which. For example, it's common to have different blocks per langauge or per node type in the same region, which may even have the same admin-title. Showing the conditions in the block on the main layout page lets users figure out which block is which. My initial version didn't have these collapsed at all, they were just a plain list in the preview, but then I found that they can easily clutter the interface because the description for conditions can be quite long. I'm not sure what to do visually with the list of conditions; a fieldset probably isn't the right tool, but I think it's important to keep them easily accessible and viewable from the main layout editor.

In order to get a "node-type" context available. You have to know it exists.

Right now there's actually no way to manually add a node-type context (or any other context). The only contexts you can use are those that map to arguments in the URL. We have the ability to add custom contexts (either based on relationships or hard-coded to a single ID) at the API layer, but it's not exposed to the the UI yet. Perhaps what you're suggesting here is that we make a list of suggested "paths", such as node/% and user/%? The context will be selected automatically based on that path. Out-of-the-box I think it may be a good idea to include a few sample layouts, potentially in the "disabled" state, so you could get an idea of how wildcards and contexts work without needing to really understanding the concept.

Selecting context per block

We might need to discuss this over a chat or video, as we might not be using the right terminology here. For the most part there's only going to be one context available for a block, with the exception of the "User" context, in which it's going to be common to toggle between the "Current user" and a user from either the URL or (down the line) a relationship. So usually selecting a context within a block isn't necessary. Perhaps you're suggesting that we filter the list of blocks by the contexts that block uses, which seems like a great idea. Views creates a "Category" for fields and filters, but it's arbitrary. If we replaced the Category dropdown with a "Context" dropdown, you could find all the blocks that leverage the "User", "Node", or "Global" contexts, which would be doubly useful to explain both the concept and to find the blocks you're looking for.

Thanks so much for taking the time to both watch the video and provide your feedback Bojhan! This has been in the works for months and I'm glad that it's finally presentable. This is only the round-one attempt, something we can use to get the APIs in place and then iterate from there. A lot of the UI tasks I've intentionally left bare-bones so we don't have to feel too attached to interfaces that are already there, with the exception of the dialog-driven options, to which we're heavily tied.

Once we get the tests passing and prepare this for inclusion, we'll need to make a meta of Layout followup tasks to keep track of all the additional features we plan but have cut for the initial release. UX improvements can be included there or maybe made into it's own meta. @jenlampton has volunteered to manage the metas, so we can expect to see those written up soon.

quicksketch commented 10 years ago

Are you planning to publish any guide for themers that come from the D6/D7 world?

I hope so, at the very least we'll need basic upgrading instructions, for modules as well as themes. This will fundamentally change some key concepts in themes, most notably you probably won't want a page.tpl.php file at all, unless you want to hard-code your header and footer universally throughout the site (which some people may want to do). It'd be more common to override the new header.tpl.php file instead, which contains the logo, site name, etc. that usually appears at the top of the page.

Backdrop will probably never allow a theme to provide layouts directly. Instead we'll make layouts themselves a new top-level concept, like a module or theme. It's likely that "themers" will also be responsible for creating the layouts the site will use, but those layouts must be separated now into their own separate template and CSS file, rather than being tied into the theme directly.

docwilmot commented 10 years ago

Fan-freaking-tastic!

behindthepage commented 10 years ago

Notice: Uninitialized string offset: 0 in layout_menu() (line 247 of /home/thepage/public_html/addons/backdrop/core/modules/layout/layout.module).

On a new install. Edited default layout and changed the setting to the Three/three/four stacked columns.

quicksketch commented 10 years ago

Thanks @behindthepage! I've run into this a time or two as well. I think I've fixed this issue in the latest commits.

Progress-wise, I've now got all tests passing over in https://github.com/quicksketch/backdrop/tree/86/layout_module. It may break again as I expand the test coverage, but we've got at least one clean Travis CI build.

I ran into some major issues with the upgrade path, where when upgrading from D7 to Backdrop block positions would not move over properly. I rewrote the update to write config files manually rather than than using the Layout API, which isn't available under update.php when doing an upgrade.

Overall we're looking really good. The primary task at hand is still testing. We need to expand the test coverage for Layout module beyond what other modules coincidentally test, at the very least replace the block tests that were removed that dealt with positioning and condition testing.

behindthepage commented 10 years ago

That is great. This is a really great module and very complicated. Well for a hack like me it is but I have other skills, I have broken it again! Well, I am still getting my site broken with any editing of any of the layouts.

Am I cloning the correct branch with this? git clone -b 86/layout_module https://github.com/quicksketch/backdrop.git

Gotta make sure I am testing the correct and latest code. Regards Geoff

quicksketch commented 10 years ago

Hey @behindthepage, yep that's correct. However after updating you may need to do a fresh install. Obviously we need to get the upgrade path fully working before this is ready to go. If you can give this another shot, I'd love to get your feedback on your experience with the module. There have been a lot of changes in the past few days and stability has been improving.

Status update on where things are now: We have tests fully passing! Yay! Most recent build from Travis is at https://travis-ci.org/quicksketch/backdrop/builds/36592375.

A significant change implemented recently is that all of the default layouts that come with core are now top-level concepts. Layouts can now stand alone by themselves, independent of a module. You just need a .info file, a tpl.php file, and a CSS file to create a layout. This should make it trivial for front-end developers to create layouts with a minimal amount of code (since the templates are still PHP). Core's default layouts now live in /core/layouts/*.

I've started building out more extensive dedicated tests in core/modules/layout/tests, which revealed a number of problems around context handling and custom arguments. Tests still to be done (at least):

klonos commented 10 years ago

...btw are there any plans for a visual d&d layout builder?

quicksketch commented 10 years ago

...btw are there any plans for a visual d&d layout builder?

Probably not in core, though we could fairly easily port the Drupal "Layout" module (we'd have to rename it of course, maybe "layout_builder") and wire it up to Backdrop's Layout module. https://www.drupal.org/project/layout

klonos commented 10 years ago

Well, if this functionality was available in core out of the box, it would make creating a custom theme a walk in the park for newcomers. Maybe consider it for 2.0.x?

quicksketch commented 10 years ago

Yeah, I think we should port the functionality first, see how well it works and how popular it is. My experience is haunted by the awful layout builder in Panels, where you would be insane to use them on a real site. Just the basics of Layouts are likely for 1.0 and we'll definitely be expanding on them as we go forward. If there's demand, there's no reason why a Layout Builder would need to wait for 2.x, it could be in a minor release, 1.4 or something.

klonos commented 10 years ago

Yeah, it would be a killer feature. I honestly don't understand why the Blocks & Layouts initiative didn't move forward for D8 (time constrains I guess), but if we manage to implement it and improve on it as we go, I feel that it will be the one feature that makes people new to the drupal/backdrop world prefer Backdrop.

Now if we only also get the rich text editor in... ;)

quicksketch commented 10 years ago

Now if we only also get the rich text editor in... ;)

Yeah no kidding. ;) After the "big 3" (CMI, Views, Layouts), it's the highest priority item, IMO. It's primarily being delayed because it can be added after the initial release (1.1 or 1.2 probably).

quicksketch commented 10 years ago

Before we go too much further, I wanted to check what kind of performance impact we're having by switching to Layouts, since they'll affect the loading and rendering of almost every page on the site.

The initial results weren't too good, it looked like Layouts were slowing down the site nearly 30% in my first passes. However, I hadn't done any performance optimization yet, including any cache tables. After adding a cache table for storing which layouts are needed at any given path, things picked up signficantly. Here are the benchmarks I've got so far.

This one is for the front-end of the site, with 11 blocks loaded on the homepage (Header, Breadcrumb, Main Menu, User Login, Who's Online, Main Content, Who's New, Administration, User menu, Powered-by). The "before" is the same site, just before switching branches and running update.php. These comparisons are based on the second page load (caches set) at a particular path, with APC on PHP 5.3.22.

front-end-homepage

So about 6-7% slower, with 5% more memory usage. That comes out to 5ms and 280KB more. Not anything terrifying, but it's not an unreasonable price to pay, even as-is. More work could probably be done to optimize this further, in particular, block caching is currently removed. So things like the Who's online block and who's new block might be getting generated manually, when they previously had been cached.

To check this again a page with fewer blocks, I also tested just the /admin path, which only has 2 blocks: Breadcrumb and Main Content. This result is as follows:

back-end-admin

Here we actually have an improvement in speed, around 11-12%. Memory still increases by 8%. I'm not sure why the memory usage difference is higher here, but I don't think the 400KB increase is going to be a concern.

My early speculation is that the base system of Layouts is at least as fast as the region system. Re-introducing block-level caching might give us an edge on the front-end where a lot of blocks are displayed on the same page.

quicksketch commented 10 years ago

I've done further optimization on path loading, making it so that all layouts at a particular path never take more than a single cache()->get(). This had a tremendous benefit. Combining with an optimization to Contextual Links, we now get an overall performance improvement on front-end pages. w00t! Same conditions apply as before, with 11 blocks on the homepage, APC, PHP 5.3.22, before and after the update.

front-end-new

An 11-16% performance improvement, with memory increases reduced to 4%. This test differs from the previous ones in that I also created homepage sample content, so the overall execution time in both tests is greater than the last tests, which didn't have any nodes on the homepage.

The main benefits come from significantly fewer calls to backdrop_render() and elimination of the $page renderable. Worst offenders in regressions come from increased calls to module_invoke(), probably for retrieving Layout and Block information. The positives outweigh the negatives though; not to mention the substantial increase in functionality.

I still haven't restored block caching, so we have opportunities to optimize this further yet.

behindthepage commented 10 years ago

Excellent Nate! Kudos and thanks.

Regards Geoff

klonos commented 10 years ago

More features + speed? Great job!!!

ghost commented 10 years ago

Love this feature! Been testing it out and am a bit concerned though about the ability to add multiple 'Main page content' blocks...

Playing with this produced a few issues such as adding a new 'Main page content' block to the sidebar, saving, snickering about the fact that there's now two of everything ;), then removing the 'Main page content' block from the sidebar but having it disappear from the Content region instead... I then dragged it from the sidebar to the Content region and saved, then is disappeared! It's still there ('cause when I re-add it there's two of everything again), but you can't see it - the region looks empty.

Another time I had two 'Main page content' blocks, removed one, then removed the other (that should not be allowed) and got a The requested page could not be found. popup.

Also, there doesn't seem to be a way to 'reset' the default layouts. That'd be nice, since I stuffed up my test site with all the extra 'Main page content' blocks.

Finally, when creating a new layout, can we make it so the machine name is generated automatically off the layout title (like content types and views)?

quicksketch commented 10 years ago

Hi @BWPanda, all great suggestions. It should already be the case that removing the "Main content" block shouldn't cause harm, but admittedly I haven't tried removing it myself. There's a safety catch for the default layout that ensures it is set. For other pages (such as node/%) you might not actually want the main page content at all, and instead choose to layout the page entirely with custom field blocks. That's not available yet, but it should be in the near future. That sort of "throw away the normal content and layout it out entirely custom" is how panels has worked forever, though perhaps we don't need to be using it as a model.

Good idea on resetting default layouts. We provide resets for most other things (Views, Image Styles) and there's no reason why we couldn't support them here as well.

can we make it so the machine name is generated automatically off the layout title

This should already be the case, as we're using the machine_name element. If it's not working, then it may have been a recent change that broke it.

ghost commented 10 years ago

Ok, so the machine_name element does work (my other issues must have messed up the javascript).

Here are steps to reproduce the duplicate Main Page Content block issues on a fresh install:

  1. Edit default layout
  2. Add Main Page Content (MPC) block to Sidebar with default options
  3. Save layout
  4. Click 'Remove' on Sidebar's MPC block (MPC block from Content region removed, MPC block in Sidebar remains)
  5. Drag MPC block from Sidebar to Content
  6. Save layout (MPC block in Content disappears, Content appears to be empty)
  7. Add new MPC block to Content with default options
  8. Save layout (all appears to be back to normal, until you refresh the homepage to see duplicate content - even though MPC block disappeared in step 6, it's still there...)
quicksketch commented 10 years ago

Thanks @BWPanda, this is very helpful feedback. I'll see if I can reproduce it and (either way) add some validation around the main content block.

ghost commented 10 years ago

Cool, I couldn't reproduce this with other blocks, so must be something specific to MPC block...

quicksketch commented 10 years ago

This problem is probably specific to both the MPC block and the default layouts. My guess is that the automatic addition of this block ends up getting saved as config. Really that's a safety net to prevent completely breaking your site, so even if we just prevent this problem in the UI via validation (make MPC block required on the default layout), that should prevent the problem.

quicksketch commented 10 years ago

@BWPanda I figured out where this was coming from and fixed it in the last few commits (plus added tests). Fixes are in https://github.com/quicksketch/backdrop/commit/4f8ee324302ab1787f5225ec608cf793e7b28f43 and https://github.com/quicksketch/backdrop/commit/e8a391a3b564319889f6b3bcbbf7bdceebe8a645.

The Travis run hasn't finished, but I think we're still at 100% tests passing.

I made a new issue for followups at https://github.com/backdrop/backdrop-issues/issues/345, as this PR is going to be huge already, but we have a lot of tasks that will need completion after the merge.

quicksketch commented 10 years ago

We're nearly pull request ready here I think. I've reviewed the diff (https://github.com/quicksketch/backdrop/compare/86/layout_module) a few times and I think it'd be good for us to start tallying up the API differences introduced here, which are signficant:

Introduce into layout.tpl.php:
$classes
$attributes

Moved to layout.tpl.php:
$messages
$title
$title_prefix
$title_suffix
$tabs
$content (contains all regions)

Remove entirely:
$action_links
$node
$is_admin

Added to everything
$base_path
$directory

Now part of header block:
$logo
$front_page
$is_front
$logged_in
$site_name
$site_slogan
$hide_site_name
$hide_site_slogan
$primary_menu (renamed to $menu)

Move to separate blocks:
$breadcrumb (new block)
$feed_icons (use the node syndicate block)
$secondary_menu (place as a normal menu block)
quicksketch commented 10 years ago

After filling out the hook documentation and cleaning up docblocks, I think this code is ready for its full review. I've already gone over the code several times, but at this point I think it's as good as I can do in a single pass. I've started down the "refactoring trap" where I'm just moving code around without changing the functionality. The reward on this change is high, overall changes (though significant) are limited by preserving the existing Block API.

The official PR is now at https://github.com/backdrop/backdrop/issues/483, where I'll update it with any changes as we prepare this for merging. As we've noted in the weekly meetings, we're aiming to merge this in the next week or two.

docwilmot commented 10 years ago

excellent. where to comment on issues i find? here, #345 or #483?

quicksketch commented 10 years ago

@docwilmot posting here would be great. If it's a comment on a particular line of code, you can comment on the PR directly in https://github.com/backdrop/backdrop/pull/483.