Implement the two-column layout for the new create content page

Problem still is that accesibility wise the submit button should come after the content on the right. So basicly it has to jump from the left side to right and back to the left in the end.

Does that make sense to you? And can it be done?

Uh, oh this does not really work:

Problem is the widths do not fit. The content (layout-region-main) has 65%, the meta which should be right (layout-region-secondary) has 35. So far, so good. layout-region-main also has 2em of padding-right, also layout-region-secondary appears to have a tiny bit of padding right.

Also the fieldset with the published information is not inside layout-region-secondary but -primary and thus does not get floated right at all.

The padding of layout-region-main needs to be in percentage values, say 2% and deducted from the region.
When fixing the widths, it still does not look quite right:

It should go right to the top of the overlay. But #page contains everything and cannot go up there because #branding that holds the breadcrumbs is in the way. Even when that is gone it does not fit...
Nothing big, but needs adressing.

Another problem is the summaries on the fieldsets that show the status (published, promoted to front page...). But I guess this one is easy as those should be gone completely.

I applied all three patches: for the Published checkbox, the vertical tabs as fieldsets and this one. Maybe there is some unwanted interaction between the patches.

Yeah I think you are right. As nice as it looks having the grey extend to the very top, this is still a node form and needs to be functional first and foremost. Avoiding five cans if worms and going for something easier to implement sounds sensible.

The patch fails to apply.
Reroll appreciated.

Now the other patch does not apply completely as well. Here my results with a fresh copy of core HEAD: http://screencast.com/t/PjJIqxlr
Are you sure you are chasing head and always do a git pull before creating the patches?
Well, the drop may be on the move quite fast these days, so it may be an athletic challenge... :P

Ah, Patch (as well as the other two) applies cleanly now. Just learnt about git apply -v patchname :)
Looking good http://screencast.com/t/4bwn1QZ1h , just as it should. Off we go. Needs someone with coding skills to RTBC though.

I was pretty happy seeing the layout system being used here, but then realised that it is not really used through the layout system, it is used from there as a template and CSS file. We can do much better than that now. You can get the layout system generate the output for you with the template + an array of -reprendered regions. For an example, see layout_page_view() in core/modules/layout.admin.inc in core:

$key = '...'; // layout key
$instance = layout_manager()->createInstance($key, array());
$regions = array('regionkey' => '..value..');
$rendered = $instance->renderLayout(TRUE, $regions);

This will add the CSS that and render the template with the rendered regions provided.

Second, I'm not sure this layout should be defined in layout module, notbut node module. Especially if we want it to be presented as THE node editing layout, not some general "two column with footer" layout.

Finally it would set a requirement for layout module on node module as well for layout module, but the reuse is worth it IMHO. As more and more things will use layouts, it will be ubiquitous.

Probably layout could be assigned with a help of #1875992: Add EntityFormDisplay objects for entity forms that part of meta #1875994: [meta] EntityDisplays

So there are some problems with using the layout module:

1. It doesn't return a render array so there's no way to alter at the very end - unless we don't render StaticLayout::renderLayout
2. The form tags aren't rendered around the layout and I don't see any other way around it at this moment - unless we return a build as mentioned in 1 - at least I think - not sure how I can call drupal_render_children() on the remaining form elements.
3. And even if we can make this all work, then there's the fact that currently the right column only happens when you're in 'admin mode' not in your frontend theme (well at at least the metadata is).

I'm not sure either to set a dependency in node in layout is interesting in case at release of D8 this is the only implementation in core - unless we move it to system where it would make more sense IMHO to live there.

I'm going to investigate a little more to see if I can workaround the problems I mentioned.

So, currently, the patch uses a simple template and preprocess function the 'old' way and it's inside the templates folder.

Patch is attached, but needs still more work re: styling for with and without overlay - it doesn't have all css either as I still have apply problems, but that's for later. Screenshot attached for temporarily reference.

Great work!

Added some theming for the overlay mode & non-overlay mode. Swentel removed the template preprocess function as it is redundant.
Still need to do a little crossbrowser testing. Attached some screenshots in Chrome.

Setting to needs review.

Screenshots look great.

In a follow-up, we might want to revisit the sections and consolidate them since it seems silly to have groupings when we virtually always have one item in the section. WOuld be good to get Yoroy thinking about that.

Tested on an iPhone emulator and mobile opera emulator, looks great! I've attached the best view I could of the create content screen at 320 pixels.

It also looks good on Firefox and iPad.

oops, here's the firefox screen

Reroll, the drop is moving fast the last hours.

#31: 1838156-30.patch queued for re-testing.

Small fix to remove the white space in the overlay at the bottom, all credits go to sannejansen

#34: 1838156-34.patch queued for re-testing.

Nice work!

As a followup, it would be nice to get the whole section header to be clickable (e.g. Menu settings) instead of just the text,

@moshe: there's (for the moment, as far as I could find) no support for the tag in Firefox and IE, so i wrote a workaround for that. Now the whole bar is clickable (not just the text). Also added some IE8 & 9 fixes.

I mean the <details> tag.

@ sannejanssen - thanks!. Without an interdiff, it is hard for me to see what you have added/changed. I do know that some work around the details element is happenning at #1839158: Replace collapse.js with a proper polyfill for <details> and we should not duplicate it here. Are we duplicating? If so, lets just go with prior patch (could you reupload that for test bot).

@moshe: the fixes are pure css. Attached the patch & the interdiff.

#41: 1838156-41.patch queued for re-testing.

One thing that's a bit odd is that the layout still shows in single-column mode even in iPad landscape mode. Is this intended?

This looks great, although I think the breakpoints can be improved a bit (see Angie's comment above).

Let's wait for this to come back green and then it can be committed IMO.

I don't think it's supposed to be like that. If it turns green i'll update the media query, unless someone thinks otherwise?

@sann - we decided recently to do anything special for IE8's lack of media query support. see #1645494: Allow core themes to display single-column on IE8: do not compensate for lack of media query support

I agree that we should have 2nd column on ipad horizontal orientation.

Ok, marking needs work then. This is looking great, btw! :D

I see. I kinda had a feeling the ie.css file was pretty empty.. I'll take it out then?
I just checked on the iPad, and maybe we can even have the 2 columns on the portrait view? I think there's still enough space to have both columns next to each other. Or is there some kind of agreement on this?

crosspost :)

Yes, remove IE8 CSS please. IE9 does media queries so I'm not sure what you are doing. It might be appropriate or maybe not. I guess ask someone who was in that issue that I linked to.

Sure, two column on IPad portrait sounds good.

Thanks.

I'm unsure about 2 columns in portrait mode (think Ipad mini for example). But don't let that hold up an initial commit, if the next patch shows 2 col in landscape this is good to go.

Looking very good. Also makes it very clear that we have some regrouping to do on what lives in which tab :)

@moshe will do.
@yoroy good point! Haven't had the change to look at the ipad mini in detail, but I'm sure your right. Let's stick to the two column for landscape then.

@yoroy I guess Ipad Mini will get a different media query that will also fit for 7-Inch tablets like Nexus 7 if it is too cramped. Dang, mobile devices have too many screen sizes.

That upgrade failure is unrelated.

Did you change the media query to show 2 columns in tablet widths (portait and landscape)?

<div class="layout-region layout-region-node-main">
+    <?php print drupal_render_children($form); ?>
+  </div>

I think that can be just render($form)

yeah, agreed that whenever we mention a specific device we're basically doing it wrong. Define breakpoints on the needs of the content is an excellent guideline. Two-column portrait seems too cramped indeed.

@ry5n maybe we should special case this two column implementation *only* to seven then ? This would remove some code in the nodeFormController then (like switching to container type and the theme etc). We're done then with a simple hook_node_form_alter() in Seven and all CSS can be moved to Seven then - I guess ?

Also the two column looks a bit weird when there's no overlay, it kind of floats to much, but I'm not sure if we want it like in #26 - http://drupal.org/files/01.create-article.png - I remember asking Bojhan and he kind of agreed that was the way to go, however I'm not sure how hard that is to implement ?

#67: 1838156--67.patch queued for re-testing.

This needs a reroll both due to corruption of the included images, and for recent changes in HEAD of NodeFormController.

@ryan are you able to reroll - I'm bad with images and stuff :/ I'll look into the messages problem after that. Note that there's a form_alter now in Seven theme that should be used to tell the node form to use the two col template. It also means that the CSS is now exclusively for Seven, so maybe some css can be moved around ?

#79 looks good for me

Yup
The binary one )

Tested it with keyboard only & ChromeVox and both worked fine.

#79 just found a small design typo

tag

I can't seem to recreate the layout issue in #86, it might be cause by the increased word length using the non-default language? Although it is posible to set styles different on a per language basis I don't think I've seen that kind of complexity elsewhere in core.

The styling of the delete button is global and we should probably handle it in #1848292: Consolidate Seven button styles

A lot of really great work has been done here, and I think it's ready enough to be committed. Once it's in HEAD, I'm sure people will discover further tweaks that are needed. I can confirm that #86 does happen when you change the Delete button label to something long enough and make your window small enough, so that's one follow up worth opening.

+++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
@@ -56,6 +56,28 @@ protected function prepareEntity(EntityInterface $node) {
   public function form(array $form, array &$form_state, EntityInterface $node) {
+
+    // Visual representation of the node content form depends on following

Didn't we just revert these changes back to the original code in the other issue?

Also, something doesn't quite seem to work with the two column layout:

Indeed, we moved this to seven_form_alter() , see attached interdiff. This patch will not influence any other theme anymore at this point besides seven now.

Follow ups I see, but not yet created:

• The node/add URL is also affected by the CSS. Since seven implements hook_preprocess_page() we can maybe add a dedicated class to the node form page so we can have better selectors.
• Figure out a better placement/styling for the error message.

We can start finishing #1751754: Implement new form style for Seven, based on blueprint mockups.. Also note that the 'delete' styling is intentional as you can see in the screenshot (unless you're referring to the background) - but is open for discussion of course ;)

Thanks. Much better. Since we're getting close to feature freeze here, RTBC. However, here's some minor feedback/questions for either quick addressal before commit or in a follow up.

+++ b/core/modules/node/node.edit.admin.css
@@ -0,0 +1,69 @@
+  /* @todo File an issue to add a standard class to all text-like inputs */

Has this issue been filed? If so, please link to it.

+++ b/core/modules/node/node.module
@@ -171,6 +171,11 @@ function node_theme() {
+      'path' => drupal_get_path('module', 'node') . '/templates',

This isn't needed.

+++ b/core/modules/overlay/overlay-child-rtl.css
@@ -22,8 +22,16 @@ html {
 #overlay-close,
 #overlay-close:hover {
-  background: transparent url(images/close-rtl.png) no-repeat;
+  background: transparent url(images/close.png) no-repeat;
   border-top-right-radius: 0;
+
+  -webkit-border-top-left-radius: 12px;
+  -webkit-border-bottom-left-radius: 12px;
+  -moz-border-radius-topleft: 12px;
+  -moz-border-radius-bottomleft: 12px;
+  border-top-left-radius: 12px;
+  border-bottom-left-radius: 12px;
+  background-color: #ffffff;
 }
 
--- a/core/modules/overlay/overlay-child.css
+++ b/core/modules/overlay/overlay-child.css
@@ -77,6 +77,14 @@
   /* Replace with position:fixed to get a scrolling close button. */
   position: absolute;
   width: 26px;
+
+  -webkit-border-top-right-radius: 12px;
+  -webkit-border-bottom-right-radius: 12px;
+  -moz-border-radius-topright: 12px;
+  -moz-border-radius-bottomright: 12px;
+  border-top-right-radius: 12px;
+  border-bottom-right-radius: 12px;
+  background-color: #ffffff;

I think this improvement to the Overlay's close button is great, but why is it included in this issue?

+++ b/core/themes/seven/style-rtl.css
@@ -205,3 +205,22 @@ div.add-or-remove-shortcuts {
+/* Narrow screens */
+/*.overlay .layout-region-secondary {
+     -moz-box-shadow: inset -0.15em 0.3em .5em rgba(0, 0, 0, .1);
+  -webkit-box-shadow: inset -0.15em 0.3em .5em rgba(0, 0, 0, .1);
+          box-shadow: inset -0.15em 0.3em .5em rgba(0, 0, 0, .1);
+}*/

Why is this commented out? Should we delete it, or is it serving some purpose?

+++ b/core/themes/seven/style.css
@@ -1523,7 +1533,137 @@ details.fieldset-no-legend {
-  display: none; /* Hide JS summaries. @todo Rethink summaries. */
+ display: none; /* Hide JS summaries. @todo Rethink summaries. */

Accidental removal of leading space?

Committed #92 to 8.x. Great work moving this forward. Setting back to 'needs work' for the issues identified in #93, and possibly one from #90.

Whooo! Thanks Dries, it looks like we might be able to get this thing changed in core.

This looks great, but what happened to the vertical tab summaries? I thought we agreed to keep them in the new design (see #1510532-148: [META] Implement the new create content page design and subsequent comments), i.e. similar to what was shown in earlier designs such as http://groups.drupal.org/files/8_add-content_b.jpg. This was because intermediate and advanced users find them helpful, and also because Drupal 6 usability testing showed problems with pure collapsible fieldsets for beginners as well (although recent tests have apparently been less conclusive).

Followup to add those back?

Nicer formatting.

At minimum, we should remove the node.edit.admin.css file as a follow-up patch here. The entire styling is performed in Seven theme.

Followup to remove obsolete IE css committed from this issue, #2021073: Remove Seven's ie.css

Additionally we should correct a deviation from the original design that occurs when overlay is disabled:

As you can see there is white space above and to the left of the right column. This could be removed by adding negative margin top and right to .layout-region-node-secondary and setting the overflow on .layout-node-form to visible:

.layout-region-node-secondary {
margin-top: -1.55em;
margin-right: -2.25em;
}

.layout-node-form {
  overflow: visible;
}

Resulting in the desired look:

But that seems like the wrong way to do this to me and I suspect it would mess up overlay. Really the right approach would seem to be removing the margin on #page and applying it instead to .content but I lack the CSS skills to make that happen. Anyone care to try this?

This is a minor fix, tagging novice.

@tkoleary The design changed to accommodate breadcrumbs, can we make sure that wraps correctly then? Other than that major plus to fixing this.

This patch addresses #100 with similar css that had been used for the overlay, lines 1745-1752 of seven's styles.css, adjusting #console and .messages.

Unrelated, I noticed for no overlay, there is still a broken breakpoint between 780px - 832px (52em) where the media query css was written only for overlay, present before and after this patch.

@echoz

Awesome work! That was quick. I'm going to ask Angie to review and mark RTBC.

Sorry, I can't review CSS patches. :) But once it's RTBC I'm happy to take a look!

This CSS looks correct. I'm a bit confused why we are printing an empty #console div on the page?

That's a render/twig bug #953034: [meta] Themes improperly check renderable arrays when determining visibility

#106 yep, this is a compensation for the empty #console div's margins.

So it should have an @todo pointing to that issue.

But with this patch applied, won't they look incorrect when they do have content in them?

The patch just moves the (top, bottom) margins from #console to .messages in this display, as the overlay css in the committed patch does.
Screenshot of #103 patch with message:

Seems like what's needed is either:

1. A change to the console region to give it a 100% width border-bottom, or
2. A change to the template that places the console inside of #content so that it is in the left column and only pushes down title and body

Yup agreed, it shouldn't push down the sidebar, it should only push down the contents of the form.

I don't think that's possible with the way that the two column layout has been implemented. We aren't using the traditional content/sidebar region method because it's all part of one form.

@LewisNyman

So if that's the case version 1: A change to the console region to give it a 100% width border-bottom.

Or even better we could make the console messages "toasters" that appear and fade in a fixed position like growl messages :)

Actually, could we split this discussion + the patch at #103 off into a sub-issue and mark this issue as fixed? We're already at 116 comments, and it's looking like this needs more discussion.

Just opened this issue to address #103 issue. Uploaded #103 patch as webchick suggested.

Proper link to new follow-up #1919956: Determine when and/or how to display summaries for vertical tabs and details ui components