-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[D8] Add ability to create configurable layout templates #3755
Comments
@docwilmot For the benefit of those not wanting/able to read through over 1,000 lines of PR code, could you please describe the feature request in more detail...? :-) |
Reasonable, updated parent issue. (edited) The current PRs UI is probably not terribly intuitive though. To create a new template, you go to the settings page for layouts module (not the configuration page for an individual layout), click add new template, give it a unique name, then you'll be directed to an interface similar to the layout drag and drop page (it's based on this, extending the editor renderer). Once you've finished adding all the rows, save, then this new template will be available in the configure page of all layouts. Use as normal. |
I like where it's going. |
I am so excited about this!!! Feedback follows :)
Other:
|
Thanks for the comprehensive review @jenlampton. I agree with all your comments. A couple of replies though:
TBH I'm not sure about the entire administrative UI for this PR, but I'm assuming you dont mind the "links within an icon" motif? But I think dropbuttons might look awful from within the icon.
I considered this, but some of the fractions arent integers if you do, like
I considered that, and it can be done, but wondered if it would be necessary, considering the region names arent exposed to the end-user.
😄 there is a list at |
Yeah, I saw your comment about that on the main issue, but I don't have any better ideas about where to put this.
Oh yeah, I hadn't thought of that...
Rounding is fine, even to the nearest whole number. It is more intuitive than 10:2, but only because 10:2 only has meaning if you know it's an abbreviation of
I noted that as a problem as well. But if we make the region names visible (and hide the row names) to match all the other layout templates, then people will want to change the region names. We could even kill the row names entirely, and use a random hash or something instead. Nobody will care what those are since they don't appear in the UI anywhere.
Ooh! I missed that. Checking it out... |
What if in addition to the ratio we have a visual showing the column widths? Regardless I agree it should be two dependent dropdowns: number of columns and column widths. |
Maybe we could have a tiny example row? I was thinking about automatic thumbnails of the layouts - but that seems complicated... |
Thanks for doing the first round @jenlampton ...you beat me to it 🙂 ...very thorough 👍 ...here I go now 😅
Renaming it to
I say yes to this, but perhaps include the more "technical" versions of these things in brackets:
I think that it's high time we switched from the tile listing to a table here. This would allow us to place the links in dropdown menus, within an "Operations" column (which is a common UI pattern we use elsewhere). It would also allow for a search/filter section at the top, which people could use to search the list of templates; and we could have filters like a select for 1/2/3 column layouts etc (I think that the D8 Layout Builder allows that too ...or maybe I am thinking Panelizer?). ...we can leave the template selection when editing/creating layouts still be a tile listing; that's fine.
See my comment above. Having a table would allow showing the description there (as well as allow for a column with a list of which layout each template is being used in).
Yes, lets please just merge the edit and configure forms into one, with the name/description fields being at the top of the region management UI (a-la #475). No need to have people jump into another form to simply rename a layout template. Some other suggestions:
That's it for now. Loving this ❤️ ...so much so, that I am tempted to set the milestone to 1.14 😄 |
Agreed, plus, once you've added new rows, they wont be necessarily at the "top" or "bottom" any more!
I think these are too verbose, and still unclear; how about:
That would work nicely, especially if it mimics the layout list in appearance, but it would be a big change, would like to hear thoughts on that!
I dont like this though, this UI borrows from the Editor renderer which builds the drag and drop block page for layouts: an initial page to create the layout and a separate page to configure it. And yes it matches other areas in core where you define a thing with its machine name, then you go somewhere else to configure it (menus, Views, etc). Plus, did I mention I dont like it? I think it would look awkward.
I considered that, and I also dont really like "flexible" but "custom" is taken: thats what you'd call templates you make and store in
Good point; could swap them to whatever "Default layout" is using, or Moscone.
Missed that. Fixable. |
Wow, I also like the PR! (My test page: http://2646.backdrop.backdrop.qa.backdropcms.org/test-page) I've only one question at the moment: While adding rows to a test layout, I was wondering if I can reorder them, and it doesn't seem to be possible. Is that by design? |
👍
These options are less clear; a margin can be anything! I'd much rather have an accurate but verbose description that people will understand, than something that's too short and ends up cryptic.
But we already have a table. and making "Which templates are allowed?" question into a giant table makes it hard to tell that it's even a question anymore.
We can leave the template selection for enabling/disabling layout-templates too. The tiles on the settings page are intended for one purpose: to enable/disable the layouts allowed, and to give people a visual cue (the tile) as to which is which. I think we should leave this interface as-is, and move all the other layout-template related things to the layout-template listing page, the one that's a table. (That includes moving the action link
I have the same hesitation about this as I do with #475. It would only be an improvement with a simple layout. The same way that is only an improvement when you menu is less than 50 links long. When you have a complex layout, or a very long menu, you don't want those other settings on the same page, or in the same form.
This change isn't necessary if we use the existing layout-template table UI for everything this form element wasn't intended to do.
I agree with @docwilmot, having another namespace conflict for layouts would be a bad idea. * For WordPress these are called
I disagree. I don't think we should take a tool people might have already been using and change it to do something else. It would be better to add another link.
Can we check and see if a layout-template is in use, and throw an error instead? I expect that this would be a mistake or an oversight 99% of the time. I'd prefer that we make people actively change their layout-template before they are allowed delete one that's in use.
I expect this can be added, but it's probably not easy! There's a bunch of great feedback in this issue already, but I want to stop us from having too many more good ideas until we get a round 2 of the PR. Otherwise, there will be too much to keep track of in these comments. (Maybe we can create a todo-list of changes in the parent issue?) |
No it won't; we'll gray the disabled ones out, and move them to the bottom of the table; same as we do with the views listing for example 😉
Yes, I will go through all comments and make a tasks/ideas list in the issue summary. For the ones that we don't have consensus, I'll add a "Decide if ..." prefix 😉 |
Perfect, thank you!
"needs more feedback" maybe? :)
I think my resistance to this idea is that right now we only have one layout setting: "Which layout templates?" ...but the current page is a layout settings page (and not a layout template list) because we were anticipating having more settings for layouts. Especially as we start to expand layouts to match more of what panels provided. Where do the other setting questions go, if we remove the layout settings form and replace it with a layout template list? Or do we remove the form now, but add it back later, when we have more layout settings? |
Looking good @docwilmot 👍 ...as always ❤️ you work! Thoughts/ideas following... |
...back to the layout settings page (layout template table):
|
Thanks for the feedback. Not sure I like the 2nd screenshot in #3755 (comment), will see what everyone thinks, but it seems a bit more complicated than I'd like. |
But I've just realised a serious weakness of my current UI: you cant edit column names in a row! 😮 |
@docwilmot during todays' dev meeting, we have all agreed that "flexible" is the name to go with, so all good on that front 👍 🙂 ...now all that's left is to pick an icon and go with it! |
The icon is also a low impact change we can swap out at any time in a bugfix point release. |
I re-reviewed backdrop/backdrop#2646 and pushed a commit to that PR that fixed code style in several places and fixed an issue with selecting the first radio button always when changing between region (column) counts in a row. However, the current pull request has issues when saving values.
|
Ah, I caused the first problem, and now fixed it. The second problem where the "Cancel" button saves the layout regions when it shouldn't is still an issue, but not a complete show stopper. I'll see if I can work out that last issue. |
Okay I think this is ready for another set of eyes again. Overall I made the following changes:
|
One more set of changes based on @herbdool's review is up. I was also bothered by the direct writing of config files in form submit handlers: when we have data structures, we should have official APIs for writing config. So I converted the I also change the pseudo-constructor to be a real one, now that we have an object. Before After |
By @docwilmot, @BWPanda, @herbdool, @jenlampton, @klonos, @olafgrabienski, @stpaultim, and @quicksketc.
Merged backdrop/backdrop#2646 into 1.x for 1.14.0. Thanks @BWPanda, @herbdool, @jenlampton, @klonos, @olafgrabienski, @stpaultim!! And a HUGE thanks to @docwilmot for spearheading this one! |
@docwilmot Found a bug in our token checking for deleting rows. Follow-up PR at backdrop/backdrop#2842 |
I got a spot check from @docwilmot in Gitter. Merged backdrop/backdrop#2842 in with his feedback. |
We should have flexible layouts in core.
Currently to create a new layout, you have to choose a template. Templates are provided in code, in a package with an info file and a tpl file, similar to modules or themes. So to create a new template requires some coding.
This PR allows creating a new template in the UI, by adding an unlimited number of rows, with each row optionally divided into preset columns. Each column a layout region.
There is another issue somewhere, but I cannot find it.
MVP (BLOCKER) TODOs:
Columns
toRegions
everywhere (for consistent terminology)Configure
link in drop button toConfigure regions
Edit
from drop-button; useConfigure name
instead (edit means db storage is used)ACTIONABLE TODOs:
Add new row
or similarChange column style
should beNumber of regions
(no style setting here)Select number of regions
option from theNumber of regions
select list; set the default to 1, and also select the first available option from the region widths.Column style
toColumn widths
(no style setting here)Selected column style
toSelected column widths
(no style setting here)Configure regions
Row width behavior
optionsadmin/structure/layouts/settings/flexible-template/add
) redirect user to flexible layout configuration page.#type = help
element to the top of the flexible layout configuration form.NICE-TO-HAVE TODOs:
admin/structure/types
Row width behavior
field via#ajax
Row width behavior
so fluid (most common) comes first, and is default.Configure regions
should use the template nameBack
buttons andCancel
links to multi-step dialogsNumber of regions
select a set of radios instead, and have them be horizontally aligned next to each other (see mockups)Region titles
fieldset to simplyRegions
Selected region widths
setting into theRegions
fieldset, as a description.Row classes
,Wrapper tag
m andRow width behavior
elements into aStyle
fieldset to match the block confgiration UI.Style
fieldset below theRegions
section (since it will be used less frequently)admin/content
NEEDS FEEDBACK TODOs:
admin/structure/layouts/templates
& leave the more general "settings" pageadmin/structure/layouts/settings
for general layout settings (like permissions)Row width behavior
optionsFOLLOW-UP TODOs:
Advocate for this issue: @klonos
PR by @docwilmot: backdrop/backdrop#2646
The text was updated successfully, but these errors were encountered: