This is a sub-issue of #1787634: [META] Decouple layouts from themes.
Problem/motivation
With all page components hopefully converted to blocks (such as page title, primary menus, secondary menus, etc) *and* the proposed possibility to create landing pages with separate block setup / placement / configuration, we need a way to control master layouts where the common elements are placed, so when you create a page with a layout, you don't need to start over and place all the common elements all over again.
Proposed solution
Ideally we would have a way to create multiple master layouts that can be used to create pages with different block setup, however, for the pre-feature freeze scope, it seems possible to only support master front end and master backend layouts, where all pages are derived from this layout. Also while we call these layouts on the frontend, the backend implementation calls them displays.
Designs for proposed solution
Listing of two specific layouts possible:
Editing of the layout:
Comment | File | Size | Author |
---|---|---|---|
#95 | 1841584-master_displays-95.patch | 179 KB | sidharthap |
#70 | interdiff-66-70.txt | 18.62 KB | frega |
#70 | 1841584-master_displays-70.patch | 165.84 KB | frega |
Comments
Comment #1
Gábor HojtsyFix naming in title. Oh these names.
Comment #2
duckzland CreditAttribution: duckzland commentedNice design, one question though, where can user add new block or region?
I've implemented a similar system in my Sigmaone base theme and utilizes 1140.css grid system. Will layout in core supports css grid system as well?.
Comment #3
Gábor Hojtsy@duckzland: the layout/template comes from a module or theme. That module or theme can use any grid system and/or make the regions editable and configurable. That is not really the role of this screen. This screen takes that template and lets people put master blocks in them (that would be useful for all pages at specific places). The huge + buttons on the side of the regions allow for block placement in each region in this design.
Comment #4
duckzland CreditAttribution: duckzland commentedso themes css is responsible to set the width of the layout item (region or block) as in the image? along with custom classes (if the theme is using a css grid system)?
Comment #5
Gábor Hojtsy@duckzland: see #1787846: Themes should declare their layouts and its changelog at http://drupal.org/node/1813372 for how this is structured.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedBy the way, see #1841824: Terminology: figure out proper words to use in the UI for describing unpopulated layout vs. populated layout to discuss template/layout/display terminology.
Comment #7
duckzland CreditAttribution: duckzland commentedahh thanks for the links, I got it now.
Comment #8
Gábor HojtsyOk, here is an initial patch.
1. OMG terminology galore. Displays edited on the UI as layouts, where they depend on layouts on the backend, and OMG anyway. See and participate in #1841824: Terminology: figure out proper words to use in the UI for describing unpopulated layout vs. populated layout.
2. I introduced a new display.module in this patch. The code added might be as part of the layout module, but that does not really matter at this point IMHO. We can move around code for months and #1839278: Add layout template demonstration is well entrenched into modifying layout module in similar ways for other reasons, so I wanted to expressly avoid conflict.
3. I've added a display_entity_info_alter() to alter in our list/form, etc. controllers. I could have modified the existing displays code as well. I guess we should discuss if this UI will be a required piece of core (in which case the references should go there) or not (in which case the references need to be altered in AFAIS).
4. It is not possible to add or delete displays. This is by design as per the design above. You have a front end master and a backend master and that is it.
5. The label() method does not work well on the Display config object. I have not been able to track this down, but that is why there are missing labels all around when you attempt to run the patch.
6. It is only possible so far to edit the layout assigned to a display, not the blocks configured. (Also the blocks are not yet plugins and the blocks we want to usually edit here such as site name or logo are not yet blocks even, so we'll need to stuff this with dummy things for a while).
Screenshots for list:
For editing:
Comment #9
Gábor HojtsyWhile doing my daily workout, I realized the label was obviously missing due to the entity keys not properly populated. Looks much better now :)
List:
Edit:
Comment #10
andypost@Gabor: idea is great, suppose we need a element and method to provide a layout list to be used on pages where layout chosen so this entity should have a render/preview view mode. Please update issue summary about related issues about page-management roadmap (spark?)
Provider detection is broken.
There's no "provider" key. Suppose we need a way to get module that provides this layout
Comment #11
andypostSuppose label should be defined, also fixed lost provider key
Comment #12
Gábor Hojtsy@andypost: I'd prefer to keep the patches involving this set of issues separately apply-able. #1787634: [META] Decouple layouts from themes has all the related issues, the provider key is set up in #1839278: Add layout template demonstration. Also, I fixed the label problem with #9. I also posted outstanding questions as to whether we want to make this module/code required, in which case the properties would go on the class, otherwise they should not go there and altered in by the module (like now). Discussing these would be preferred :)
Comment #13
Gábor HojtsyHere comes an interesting update with two new things: layout hot-swapping demonstration and block additions/move around interactivity. The layout hot-swapping is gracefully degrading, the block interaction not at all.
1. Added a layout demonstration feature to the display configurator. When you change the layout, the demonstration reloads with Ajax. (None of the modified config you made before that is saved or kept in any way). The server side uses layout remapping with the Display so it should move around any blocks that exist (although in this example, none exist yet).
2. Added *prototype* styling from @tkoleary, see the CSS. There are many selectors cancelled out now with "z#...", those are not currently used. Left there for easy referencing.
3. Added *prototype* JS code to make adding blocks work. Since we don't yet have blocks as plugins (however it is just around the corner), this code just adds a div appropriately formatted for our block presentation with a random label. The popup for block configuration will come from the blocks as plugins patch: #1535868: Convert all blocks into plugins
4. Added *prototype* JS to move around the resulting regions using simple jQuery UI sortables. There is nothing easier than this :D You can drag blocks around inside regions or across regions as you wish.
Note that *there is no code yet* to actually save these "blocks". Because the display system wants blocks as plugins, we'll need to continue mock them until they are committed. So I think we'll keep working with these pseudo blocks, so we can figure out the remapping, etc.
So once again, in short you can add "blocks", move them around, the regions of the defined template are being demonstrated properly BUT you cannot save it, and you will loose your blocks when you switch to another template.
There is also a Display error on the page, caused by internal problems within the Display system. I think this is pre-existing to my patch. I did not find any code in Display's that would let me set the blocks through the API and they would default to arrays in the abstract class, but not actually defaulting to that when instantiated. This needs to be explored in more detail.
Comment #14
Gábor Hojtsy(Take the second/bigger file from the previous comment, for some reason the smaller one was not properly removed from the post, it is missing some pieces).
Comment #15
Gábor Hojtsy- Changed all underscores in CSS selectors to hyphens.
- Wrapped JS code in once() and used context specific find()s so JS actions are only tied up once (with previous patches if you swapped templates, the add button was attached twice, etc).
Comment #16
Gábor HojtsyWorked on actually serialising the resulting block information in the blockInfo structure expected by displays:
- Added a generic textarea to the form that hold JSON to communicate the changes back to the server (not one by one as it happens so the server is not overwhelmed)
- The JavaScript *badly* *needed* predictable class names for layouts (and some predictability in layout markup) to make this work; so modified/fixed the existing layouts to have a common set of region classes
- The regions/blocks info list is paired up with the region type info on the submission side and the blockInfo array is produced as needed
HOWEVER I could not figure out why the blockInfo cannot be made to save, so I did not reach to implementing loading back the existing blocks and displaying them (since I could not make to save any).
Comment #17
sdboyer CreditAttribution: sdboyer commentedwe need to communicate back step by step, and use TempStore to save the in-progress object. JSON is an OK temporary measure, but this is exactly what the TempStore is for.
working on an in-depth review.
Comment #18
Gábor Hojtsy@sdboyer:
Well, I naturally prefer to post patches on notable steps and not dump everything at once on people, so they understand the pieces and the thought process. That does not seem to work here since I'm not getting reviews though... :/ Anyway I believe we'd store the built display object in tempstore on the server side, so we need to figure out where and how we build it, and a possible set of tools for that are in the patch. I do assume for example that configured blocks via the modal will be saved permanently, and not in the tempstore or we'll need to reference temporary blocks from a temporary edited display, which does not sound like fun, given they are independent objects. And if those blocks are saved, I'm not sure of the value of tempstore here, since you'll not be able to view the display live anyway until you save it, there is no preview functionality even on the northstar (ideal goal) design. Is the only reason consistency on the editing backend?!
Looking forward for your review and help in fixing the display save problem.
Comment #19
sdboyer CreditAttribution: sdboyer commentedok, more review. i think we're making good progress here.
if we're going to add a displays module, then we should probably move all of the displays code that's currently in the layout module to that.
i'm working on fixing the issues with saving now, but putting this review up for some discussion while i do.
we haven't yet put the logic in place that would, at install time, convert unbound displays to bound displays based on some global default layout, but once we do, these should probably be unbound displays.
This should probably specify BoundDisplayInterface.
This should specify BoundDisplayInterface, i think.
This can only be a stopgap at best. We can't have just one entity list controller for all displays, as they can serve any number of different purposes. If that means we have to further differentiate the config namespace on something like a per-use-case basis (e.g., "display.bound.page." for this use case), i think that's totally fine.
Comment #20
Gábor Hojtsy@sdboyer: (I removed two duplicate pieces from your comment after carefully checking it was probably a dreditor issue). For your comments:
1. On display module, I used this because I have similar changes in layout module for the demonstration at #1839278: Add layout template demonstration. It has been RTBC for 3 days. Once/if it is committed, we can move all this to layout module. I wanted to have patches that independently apply so people can test things without complicated dependency chains.
2,3,4. Agree with these, but the installer feature is not implemented, so it is not implementable on our side either ATM.
5. For the last one, that sounds interesting...
Are you advocating NOT implementing this as a config list controller or inheriting a class for master displays only, so we can list them here? As it is now, each (config) entity can only have one list controller class assigned (with one listing URL bound to it). I specifically altered in these so that we can discuss whether to make those changes to the class annotation itself or not.
Comment #21
sdboyer CreditAttribution: sdboyer commentedyes, i'm advocating not implementing them as a config list controller. we're designing some patterns in core for how displays will be used, but we need to expect contrib will create others. as you point out, there can be only one list controller; therefore, nobody should use it. we might reuse some of the same patterns, or even classes (which would set a good pattern for contrib to follow), but we shouldn't bind it so tightly to the entity type.
Comment #22
sdboyer CreditAttribution: sdboyer commentedi'm not sure what i'm doing wrong, but i can't get the visual thing to come up, at all. i wonder if it's some misapplication of the binary data in the patch...
can we put this in a branch somewhere, maybe in the scotch repo? it'll make it easier for me to collab.
the reason why blockInfo wasn't saving is because of this:
this doesn't work for displays, as a number of the properties that need to be written out are protected. the solution here is our own storage controller...which i want to do anyway in order to reduce some of the dense logic on the default
ConfigStorageController
(holy...crap).i'm opening a separate issue to fix this, as i think it's better kept separate and get in quickly and easily: #1849480: Allow ConfigEntity classes to control how they are saved.
with that addressed, though, we still aren't writing out blockInfo properly, because it's not being set up quite right. here's the issue:
blockInfo is not a region-sorted three-level array. it's a two-level array; the first level has keys that are block config names, each of which contain an array with that block's config info. what you want here is
$display->getAllSortedBlocks()
...sorta. in the current code, that just returns the block config names as an indexed array, ordered correctly for output in the region. but it doesn't include the info; the expectation is that client code will assemble that together. the motivation for this was that block info ordered by region is neither the final storage format, nor what's most useful at render-time (since we do not necessarily render each block in sequence by region).however, it's clearly complicating client code unnecessarily here, so i've made that method return the full set of data you want here :)
now, some further review points:
we have to use EntityInterface instead of DisplayInterface to avoid a strict warning on compat with EntityFormController's signature, but we should should add an instanceof check immediately inside to ensure that $display is a DisplayInterface.
...assuming we stick with using these classes at all, per the other line of discussion.
this means we remap on initial form build. if we have a built, bound display, then there's no reason to remap right up front.
remapping to a different layout should be a user-mediated process that happens out of band with the normal form. i wrote a big email about this to Kevin about 10 days ago, did it make the rounds with everyone?
this 'provider' value isn't set by the layout plugin itself, and it's tossing out a bunch of errors on the list interface.
are we expecting it to be added by plugin processing in the layout manager? b/c it's not, currently.
Comment #24
Gábor Hojtsy@sdboyer:
- the provider information is added in #1839278: Add layout template demonstration which has been RTBC for 10 days but got no committer attention yet
- what is wrong with the "visual thing"? can you provide a screenshot? there is no such URL is just errors, or?
- as for the use of remapToLayout() the form has a layout template selection dropdown, which when changed will rebuild the form with AJAX; I've been following the AJAX form pattern in that you'd need to build the whole form with the latest data at all times instead of relying on external data (given that the data might change the form); one such change here is when a different layout template is selected in the form; I did not expect for it to be a problem for a display to be remapped to a layout that is it's existing layout already; should people need to check outside calling this method if the layout they want to remap to is different to what it is already or is it better to improve this method to skip doing stuff in that case? :)
Comment #25
Gábor HojtsyTook a crack at fixing the test failures. Now that more complete arrays are returned, we need to check for more complete arrays. But then assertIdentical just uses === which does not do recursive array comparison, so although the error message shows the arrays compared are exactly equal in content, this patch will still fail the same way.
I did not want to make up a recursive array identical assert myself for this one, so decided to post this instead and ask for feedback as for the best way to check this. It is also likely that we are testing too much if we test the identical array structure.
Comment #27
Gábor Hojtsy@sdboyer extracted some code to make the display saving work from #1849480: Allow ConfigEntity classes to control how they are saved which turned out to needing more discussion. Including that and rerolling now that #1839278: Add layout template demonstration is committed. I'll work on making the tests pass and integrating the code with layout module now.
Comment #28
Gábor HojtsyGiven that #1839278: Add layout template demonstration got committed, I rolled this code into layout module (instead of display module). This cleaned up some naming things. Also now there are no patches other than this to make this work. @sdboyer's code ensured that saving now works well. I put in some more sample data (which shows off some of the problems of the UI), so now you can go and edit and save displays.
Yes, technically it does not use tempstore yet, the hotswapping of layouts works somewhat (existing blocks are remapped) but the reorganization will not work (due to AJAX/DOM/JS mixups when the form is regenerated). So more help / work is needed here even for the basics and we are not even there to remove blocks, etc.
(No interdiff because I moved around almost all the things).
Layout list:
Layout editing:
Tests still not fixed, doing that next.
Comment #30
Gábor HojtsyUpdated tests as per discussion with @sdboyer to test only the array keys of returned values. Using the region specific API for simplicity. Should pass tests now.
It would be great to have credible JS/CSS/AJAX expertise rolled on this patch to make the front end actually work better. I can continue dabble in some basic JS/CSS but that will probably not lead to widespread cheering.
(UI/capabilities unchanged from #28).
Comment #32
MustangGB CreditAttribution: MustangGB commented#28: In the "Layout list" screenshot Two column is under the header Template, but in the "Layout editing" screenshot the label of the dropdown is Layout. I think for consistent UX these should both be the same, i.e. the "Layout editing" label needs to be changed to Template as well.
Also (this might already be the case, so please tell me to STFU if needs be), but on the region objects within a Template declaration is there (or can we add) a simple property to say whether the blocks added to this region will be displayed vertically or horizontally. We could then lever this information in the "Edit layout"/"Add blocks to regions" page to give a better representation to the user how their page will look.
Comment #33
Gábor Hojtsy@akamustang: good catch on that terminology problem. Fixed that.
As for the vertical/horizontal placement, placement of the blocks within the regions is a whole minefield. The actual block content in itself might dictate some width requirements / constraints that the display will need to take care of. I think the thinking is that "block styles" will solve this but not in core. Core will just tack them under each other exactly like currently in Drupal 7. We need to pick battles as to what we attempt to solve in core.
Comment #34
frega CreditAttribution: frega commentedI'll have a go at converting the involved JS to backbone - there seems to be an ample amount of client-side logic involved in this. A little mv*-structure would be beneficial, imho. Initial battleplan: http://goo.gl/expgJ - i'll try to post a patch later today.
Comment #35
frega CreditAttribution: frega commentedFirst stab attached. This is a very much WIP. Drag'n'drop, adding and deleting (random id-blocks) works but switching templates currently breaks things. I shoved all the backbone stuff into one file, grunt & build comes later.
Attached patch incorporates #29/#33.
Comment #36
Bojhan CreditAttribution: Bojhan commentedCould you add screenies? Makes it easy for the non-devs to follow your progress :)
Comment #37
frega CreditAttribution: frega commentedOk, tried again.
Comment #38
frega CreditAttribution: frega commentedNow slightly more complete (if not a lot prettier)
- Modal/Dialog integration - add new block instances, Remove/"Configure" existent block instances
- Drag and Drop, across regions
- Saving works
- Split up files & grunt included
Switching Templates doesn't work yet either.
Hope the patches are ok, trying the second approach to interdiffing now :)
Interdiff.txt is obviously #33.
Comment #39
Bojhan CreditAttribution: Bojhan commentedLooks like this is in "needs review" ? :)
Comment #41
ry5n CreditAttribution: ry5n commentedPitching in where I can by reviewing the CSS.
add-block
class?:active
pseudo-class might be used instead.More generally:
-webkit-gradient(linear…)
syntax.-o
prefix.-{prefix}-linear-gradient(top, …)
for prefixed andlinear-gradient(to bottom, …)
for official.#display-blocks .layout-block
will only ever appear inside#display-blocks
. If it or something that looks similar might appear elsewhere, consider un-scoping it as a single re-usable class, something like.admin-layouts_layout-block
(or something less verbose)..admin-layouts_block
. Then add a second class and style rules to create a variant that 'extends' the base class:.admin-layouts_block-master
extends.admin-layouts_block
.Kudos to everyone working so hard on this! :)
Comment #42
Gábor Hojtsy@ry5n: the z chars are inserted to mark selectors which are not currently in use but were int @tkoleary's original HTML/CSS prototype. They are not accidental :D
Comment #43
ry5n CreditAttribution: ry5n commented@Gábor Oh! I haven't seen that before. I guess it's a case of “You learn something new every day” :)
Comment #44
frega CreditAttribution: frega commentedA small reroll of #38 to fix a reordering/rendering issue. There are still remaining issues (e.g. switching templates doesn't work, dragging onto an empty region doesn't work, etc.) and this there is no visual improvement/UX love in this, so far.
Before doing more substantial JS-work (or screenshots or in-depth reviews) on this, I think though, a little more time on the defining the architecture might be needed:
Comment #45
Gábor Hojtsy@frega: I think we do need to define the scope of this issue ourselves :) I think at a minimum, we should be able to configure these two master layouts (no other master layouts allowed for now) the configuration meaning we should be able to place (add and/or move around) and remove blocks as needed. Locking blocks and other extra features might or might not go into this issue. Instead the core of this getting in core would be to figure out how to make it accessible (in reference to previous brainstorm, an idea is to have a "show weights and regions" link at the top and make dropdowns for both appear in all blocks, so those not being able to move around things can do block reorganization).
Comment #46
frega CreditAttribution: frega commentedNext iteration, even more work in progress. Signifcant new feature is persisting changes via RESTful webservice and TempStore.
- TempStore integrated (but not 100% complete, all very new to me incl. some "blind" copy and paste from views_ui) .
- New menu loader function %layout_master_cache
- (Temporary) Webservice on admin/structure/layouts/manage/webservice handles POSTs from the BB-App (non-FAPI-based communication)
- Changes are temporarily persisted in TempStore and then "saved" properly on clicking "Save" (no revert available atm :).
- Activated a RegionModal to prepare for region-based configuration Form.
Open issues remain (e.g. switching templates flaky, dragging onto an empty region doesn't work).
Comment #47
frega CreditAttribution: frega commentedThis should now mimic views_ui's usage of temp store. Includes cancelling unpersisted changes and locking as well as breaking of other users locks.
Open issues remain (e.g. switching templates flaky, dragging onto an empty region doesn't work).
Comment #48
Gábor HojtsyComment #49
Gábor HojtsyReviewed the functionality of the patch first:
- The tempstore function seems to be working really well, it feels effortlessly quick and if I navigate away I can come back and see the same unsaved changes. Cool.
- I see you moved the big "+" away from the right side of the region. I believe it was specifically designed to be that big and prominent and loosing that seems to be unfortunate.
- I've also seen you added a configuration gear on the region, which is not necessary at all. I don't know what you'd configure there, the current functionality specs for Drupal 8 definitely do not have anything to configure there. The region type which is the only property of a region besides its machine name and label is predefined and cannot/should not be changed I believe.
Next up is a code review, although I'm definitely not the best person to review involved JS code :)
Comment #50
yoroy CreditAttribution: yoroy commentedLets summon nod_ for the javascript then :)
Comment #51
Gábor HojtsySome hopefully useful comments on the code:
Do we have doc standards for grunt.js files? A header comment would be great to summarise what this is about. There is a general lack of helper comments on other files as well :)
What do you mean?
Way better than hardcoding HTML stuff :D Cool. Are we using this phpdoc standard though in JS or was just autogenerated in a wrong format?
Why? :)
What does this locked property mean?
Looks like this is not actually 300ms or I'm misreading the code? Is 300ms (or 100ms) enough to avoid overruning HTTP requests? How is that avoided if the request time is longer?
As said above, I don't think we have any use for this one.
The documentation is not very helpful :)
Is this protected in any way from scripted attacks like forms with one time tokens? Would that kill the navigate-away possibility for the form? Any other way to solve this not being an easy attack point to screw up your site layout?
!age and !break should be @age and @break.
ha, what? :)
It does not seem to be related to the display itself, since it does not know / handle locks(?)
Comment #52
Gábor HojtsyCross-post.
Comment #53
frega CreditAttribution: frega commentedThanks for the review!
There are still some superfluous XHR-requests on reordering going over the wire, but that should be easily fixed.
I spoke to Bojhan about this in IRC. We can switch back to that behaviour/design later on, but it would require changes to the structure of HTML and the CSS and I wanted to avoid that in the initial stab.
Easily removed. It might make sense to keep this, though, if e.g. we need to provide a (more accessible, more mobile etc.) traditional form-based "reordering interface" for the block instances.
Comment #55
frega CreditAttribution: frega commentedOk, reroll. I hope i addressed everything in the review.
Additionally: made sure that there are @file blocks in all JS and fixed a few small issues/typos etc. Bugs/lacking features mentioned in #47 still apply.
Comment #56
frega CreditAttribution: frega commentedSetting to "needs review" (although it needs work as well ...)
Comment #58
Bojhan CreditAttribution: Bojhan commentedI will work on some designs for the issue raised by Gábor,
Comment #59
nod_*appears in a puff of smoke*
I had a quick look at the code, there is still hardcoded HTML that isn't in Drupal.theme.something.
More generally, that's a lot of JS for something that is essentially similar to dashboard with modals and stuff added to it. Lots of boilerplate code too. I could use a walk-through to understand the real value of using backbone here.
Comment #60
jessebeach CreditAttribution: jessebeach commentedAmazing work so far frega.
nod_, my understanding of this work is that we're moving towards a UI that can configure static layouts and then eventually responsive layouts. My intent was to move the responsive layout builder to Backbone eventually. This work will dovetail nicely with it.
I went through and refactored the CSS to match our .base and .theme standards. This involved changing some class names and therefore some of the templates. I tried really really hard not to break any functionality. I'm sorry if I did.
One other thought, in region-view.js, the render function
I don't understand why the collectionView element is being set in this region's render function. If collectionView is a container for multiple regions, would it not be better to create the instances of region views in the collectionView?
Comment #61
frega CreditAttribution: frega commented@nod_: there are HTML-chunks in the modal-views but those should and probably will be replaced very soon w/ FAPI-forms loaded from the server - so i didn't bother with Drupal.theme-ing those. Please also note: we currently parse the HTML generated on the server to generate Models and Collections; I'm going to replace that as well - but I am not sure how comfortable people are with pages that render entirely on the client-side - what's your take on that (raises all kinds of "extensibility issues")?
Regarding the overall meaningfulness of using Backbone:
First, it's sort of a matter a principle: I don't think that we should store state in the DOM, ever. Serializing and deserializing from and into the DOM does not "scale". jQueryUI-widgets IMHO are not fun to work with (ask jesse or wim). Backbone can be verbose and definitely entails more "boilerplate", but it provides a saner and more decoupled approach than a DOM/jQueryUI based one.
Secondly, framing this as a "simple dashboard" underestimates the objective and implicit complexity both in terms of user interactions and of state/models. The objective, to me at least, is to start with a working prototype that can solve the (simple) use-case of "Block" placement for "Master Layouts", and in doing so lays meaningful groundwork for similar "GUI-challenges" down the road (FieldUI's "manage field" and the responsive layout builder jesse mentions).
I'd be happy to walk you through the code - if you have categorical concerns (there are plenty imaginable) or if you don't see how it "jives" with the overall strategy in D8, please tell me: I would not want to pursue this approach (or much spend time on it) otherwise.
@jessebeach: thanks for the adjustments, couldn't find any regressions so far.
Regarding the repeated setting of the nested _collectionViews' DOM-element - this is simply needed because at initialize()-time the containing region hasn't rendered yet and thus "
" hasn't been painted. Or do you mean something else?
Comment #62
jessebeach CreditAttribution: jessebeach commentedI, personally, am OK with a UI generated completely by JavaScript for this component of the admin experience, especially because everything we're doing can be done through code if one really doesn't want to have JS enabled in their client. Sure, it wouldn't be convenient, but neither is building a complex UI that falls back to a non-JS functional state for a marginal minority of folks. We can also address aural and keyboard access in a complex GUI. There's no need to think about a non-JS version as the accessible version. Accessibility is what we make of it.
I'm not 100% solid on what I'd recommend here yet. My gut feeling is that children of a container should not necessary know anything about what contains them. The container should mediate communication to the objects it contains and be involved in their CRUD, rather than the other way around. The contained objects should only be concerned with their very focused tasks. In the case of a Region, that would be the task of managing blocks. Does that make sense? Maybe I just need to propose some code and see if what I'm babbling about makes sense here.
Comment #63
frega CreditAttribution: frega commentedHad a little idle time so tried to improve on this.
- Switch from DOM parsing to passing JSON to instantiate Models and Collections from drupalSettings.
- Fixed minor regression in #60 (region-view.js configure click event caused page reload)
- Replaced RegionsView with a UpdatingCollectionView (currently RegionsView has no distinctive logic)
- Improved UpdatingCollectionView by adding the nestedViewContainerSelector-option which allows to specify into which sub-element of a view the UpdatingCollectionView should "render"
- Added a exportGroupedByRegion method to Display to allow stripping DisplayFormController from assembling that data.
- Cleanup code in places
Switching between Templates and dragging to empty regions still doen't work.
Big questions for me are:
a) decide on whether this makes sense to pursue
b) figure out how to best load FAPI forms into dialogs
c) Leverage Drupal's new router to be more restful, refactor Webservice and introduce some form of CSRF protection
Comment #64
Bojhan CreditAttribution: Bojhan commentedJust trowing an idea in there, this removes the large plus, to a small one. I changed some of the visual styling so it still stands out. As I used the initial design, I found the large plus a little uncomfortable it takes up a lot of space when you have a more crowded layout and gives the illusion, there is another zone.
Comment #65
frega CreditAttribution: frega commentedThe current prototype looks sort of similar to what Bojhan outlined. I enabled overlay temporarily :) and took a few snaps.
Initial:
On clicking + an unstyled dialog with random blocks pops up.
If you modify the layout by moving, adding or removing block instances, changes are store in TempStore immediately and a views-like change notification is shown (yes buttons should change colors probably, too)
Please not the "gear" icon on the region is temporarily placed there and unstyled. In the "Layout Master" it doesn't have a purpose, in other related GUIs a configuration on a "container" level would probably make sense.
Comment #66
tim.plunkettWhy can't this just be:
keeping the exception and all of course, but this is a bizarre workaround
Comment #67
frega CreditAttribution: frega commentedAnother iteration, now switching templates and dragging to empty regions works, removed some duplicate requests to the webservice.
I had to hack my way around some limitations (or design?) of FAPI/Drupal.ajax related stuff (see DisplayFormController.php:88-89 and layout.admin.js:72-73 - could also be due to the fact that i don't fully grok either).
Still missing loading FAPI-forms into the dialog.
I have left the issue raised in #66 unaddressed because I know nothing about that part of D8.
Comment #68
nod_So I reviewed the code this week-end.
To me having the jquery sortable properly configured and some ajax calls here and there would do the trick. sortable has code to serialize DOM elements to a sorted list and publish enough events to react to most/all things. Also storing the state/config of the blocks might end up necessary. It's possible we'll want to add the block thing to wysiwyg and the only way to make copy/paste work properly is having all the data on the DOM element.
I'd favor a more direct approach, it's currently more complicated than it needs to be.
Comment #69
frega CreditAttribution: frega commentedHi nod, thanks for the review.
Re: 1. do you refer to the same request going over the wire twice (#67 removed only "some duplicate requests") - that can obviously be rectified but seems a somewhat pointless exercise right now.
Re: 2. there is no "no-js fallback" as the whole thing doesn't work w/o Javascript at all (#64) :)
Re: 3. and the general gist of your review - it is certainly possible to do this w/o Backbone using e.g. jqueryui widgets, drupal.ajax, FAPI and some events. I am not unaware of the boilerplate, complexity and the "impedance mismatch" between REST and FAPI, but I wouldn't want to pursue a "some ajax calls here and there"-approach. Especially if reuse is intended.
The patch is still far from that a properly decoupled approach using backbone and webservices on top of a proper API, would make sense imho - whether that's "core" stuff and what implications that has (maintaining extensibility via modules/PHP and JS) is though above my paygrade :)
As to 4.-5. that's hard to respond to without concrete examples, but please bear with me a little bit as this is only my second stab at D8 and grokking D8 can be quite a mouthful :)
Comment #70
frega CreditAttribution: frega commentedOne last iteration - now with backbone and Drupal.ajax :) It now includes a FAPI-form loaded in to a dialog. The prototype - if very ugly in appearance and parts of the code - can now do CRUD: add new blocks, move them around, update the block instance "config" (for want of meaningful things to configure just the block label) and delete block instances. Layouts can be switched on the fly (even if the logic for re-sorting them isn't very helpful).
The patch now *deliberately* combines Drupal.ajax and Backbone. Drupal.ajax is used for the block instance configuration modal (click on the gear that appears when you hover a block instance). Backbone handles adding, deleting (click the ugly "-" button), moving the block instances as well as the rendering. Please note how the two intertwine in js/layout.admin.js:7ff - I find that considerably more compelling than Drupal's traditional AHAH/AJAX stuff, but that might/will be contested :)
I don't think it makes a lot of sense for me to continue (and finetune) beyond this point without knowing whether to continue w/ Backbone or rip it out and replace it. I still think it is worthwhile to think what a "client-side driven approach" (based on a js-client backbone) could give us in a variety of use-cases, but am happy to roll either way.
Comment #71
sdboyer CreditAttribution: sdboyer commentedsorry i've been absent from this for a while.
first, a general note - in theory, i'm excited about backbone, because the DnD block placement interface is one that i expect we'll want to see lots of different versions of. we certainly could do something simpler using what jQuery UI provides us - indeed, i did do just such a thing in contrib, albeit quite crappily. but if using backbone means creating an API that will be easier to repurpose both because:
a) people know backbone, and it'll be easier for them to grok.
b) backbone provides API layers that make it easier to reuse and repurpose what we've put in core.
i don't know backbone for shit, so these are completely generic statements about pros typically associated with using a framework.
my expectation is that this UI will be re-done not on a site-by-site basis, but by a few modules in contrib and especially in distros. which means that our API target should be more skilled developers, and creating a well-structured environment that eases the process of figuring out what differentiates a given implementation...and maybe even some kind of interoperability.
@tim.plunkett #66 - since #1849480: Allow ConfigEntity classes to control how they are saved went in, it seems the separation of concerns question has been decided. this patch can now be refactored to remove the custom workaround, since it's now in the base config storage controller.
Comment #72
Bojhan CreditAttribution: Bojhan commentedSo what is this blocked on?
Comment #73
nod_1. is definitely in this issue, 2. can be split of for later probably, it'll take some elbow grease to make 1.
Comment #74
Bojhan CreditAttribution: Bojhan commentedAwesome, sounds like a solid to do. I have created #1871746: Add modal block browser
Comment #75
larowlanwill try and look at 1 next week
Comment #76
svettes CreditAttribution: svettes commentedHia! Just wondering if there was any update on this? :)
Comment #77
Bojhan CreditAttribution: Bojhan commentedLooks like this won't happen for D8 either, its quite a massive task still.
Comment #78
sdboyer CreditAttribution: sdboyer commentedwas just talking about this today with Shannon and Kris. not doing this means is pretty much saying that we have to throw all of SCOTCH out, and my impression elsewhere is that we don't want to do that - given that we're now at the point of being able to build and render a display out (at least in patch form - #1812720: Implement the new panels-ish controller [it's a good purple]).
basically, this is a required conversion if we want any of SCOTCH to be viable, which is required if we want the ESI/structured page model/promise of WSCCI. so, i'm switching this back to 8.x and calling it a task. let the argument begin :)
Comment #79
Bojhan CreditAttribution: Bojhan commented@sdboyer Basically you have to take this with the maintainers, it is my impression that you guys have and still are focused on API fundamentals. There is not enough man power and or vision, to provide a good UI for this. Even if we do manage to hash out something, it will still not match an MVP that is desirable for users - which as we already researched goes beyond the simplistic approach taken here. I appreciate your sturdiness, but its been my impression for the past few months that Spark already threw in the towel and with that the largest resource available to making this happen.
Comment #80
sdboyer CreditAttribution: sdboyer commentedyep - a conversation to be had with the maintainers, indeed.
the reality is that we're going to be tweaking and improving the API for some time to come. there's no denying that. what's different now is that, once we can get controllers in, anyway, the pieces of the API that this UI would be reliant on are taken care of. there's a much simpler conversation that could be had about this than the one we've had in the past.
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedI think it's appropriate to keep this as an 8.x task issue for now, but my hunch is that the scope needs to be significantly reduced to something like:
- Keep layout.module (or the concept of layouts as plugins, even if that concepts moves to a non-module location).
- Once #1812720: Implement the new panels-ish controller [it's a good purple] is complete, ensure that the entire page.tpl.php rendering pipeline is using layout plugins + display configuration entities instead (this is an implementation task, not a UI affecting one).
- In core, provide a preset 1:1 mapping between theme and layout (i.e., each core theme provides 1 layout plugin and Display entity) (this is an implementation task, not a UI affecting one).
- Ensure there are no critical UX regressions of admin/structure/block relative to D7: #1875252: [META] Make the block plugin UI shippable.
What this issue originally set out to do, however, in terms of a UX for adding displays, or configuring them with a radically better UI than D7's admin/structure/block, seems out of reach for core. Though with the API support reflected in the above, D8 Panels can shed a lot of its former baggage.
Comment #82
Gábor HojtsyYes, that sounds like abou the scope possible to achieve still. Definitely nowhere close to what the issue summary currently depicts.
Comment #83
EclipseGc CreditAttribution: EclipseGc commentedsdboyer and I have discussed the fact that we need to get a summary of what we think the way forward looks like, but given the limited feedback we've gotten on this issue already I feel pretty positive about where everyone is at in relation to where Blocks & Layouts looks like it's going right now. We'll get a blog post up detailing what we think is left and how that all jives with a post-18th world in the next few days. Thanks for this feedback thus far, it's greatly appreciated.
Eclipse
Comment #84
podarokany update here?
Comment #85
xjmFor reference: http://groups.drupal.org/node/287563
Comment #86
andypostThere's a much simpler implementation proposed in #1875252-47: [META] Make the block plugin UI shippable
Comment #87
xjmtemporarily tagging. :)
Comment #88
webchickThis seems to be D9 at this point.
Comment #88.0
webchickUpdated issue summary.
Comment #89
catchNot impossible to add in a minor version.
Comment #91
andypostComment #93
tkoleary CreditAttribution: tkoleary at Acquia commentedPatch needs re-roll
Comment #94
andypostcall for contrib
Comment #95
sidharthapReroll patch as per #94