Problem/Motivation

Since we have made the node edit form two columns the same logic behind doing that—reducing scroll, simplifying experience, progressive disclosure— also applies elswhere, for instance here in place blocks modal.

Following the beta guidelines the issue qualifies for inclusion in 8.0 since:

  • It's not a feature
  • It's unfrozen (CSS and markup)
  • It's a prioritized change (usability)
  • It's impact outweighs disruption. No disruption, it's an isolated change.

Currently even with only core default form items the modal extends below the fold on most desktops.


Current appearance

current

Proposed resolution

Proposed changes include:

  • Expanding the default width of the modal to accommodate the new column
  • Removing extra padding around the column tabs so that they "mortice" to the edges of the adjacent divs
  • Removing extraneous styling from the column to simplify the experience and reduce unnecessary noise (this also has a WCAG benefit in that the open tab background becomes a darker color)
  • Using an accordion behavior so that a) the modal does not expand to force scroll again and b) we remove the awkward behavior where a collapsed set can be in focus while an adjacent uncollapsed one is not in focus.
  • In order to maintain scannability when going to an accordion we add back the summary text from vertical tabs, as well as some of it's styling.
  • Additionally we move the region setting to right below title since it is the primary action of the modal.

User interface changes

Normal state

normal


Expanded and in focus

expanded focus

Expanded and not in focus

expanded not focus

API changes

This makes #2061863: Make two column node CSS reusable even more important.

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions Yes
Design review
Create a patch Instructions
Add automated tests Instructions
Draft a change record for the API changes Instructions
Improve patch documentation or standards (for just lines changed by the patch) Novice Instructions
Manually test the patch Novice Instructions

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
129.01 KB
137.6 KB
4.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,037 pass(es). View

Here is a first attempt.
However, I'm honestly not sure if this is an improvement.
Because this will usually be opened in a modal, splitting the available width between two columns leads to it being rather squished.
In addition, we lose the "summary" for each tab, like what roles or content types it is visible for.

Current in HEAD:
Screen Shot 2013-09-01 at 1.59.18 PM.png

With this patch:
Screen Shot 2013-09-01 at 1.59.20 PM.png

Bojhan’s picture

I agree with tim here, that the benefit seems quite low. Its visually a little cleaner, but because it is such a confined area it looks rather cramped. The veritcal tab, allows for at least vertically a more focused UI. Sidebars tend to work really well when the main column is long, when its short - like this, the benefit is lower. I also have no clue how this stretches to a pattern across core/contrib.

tkoleary’s picture

@Bojhan

I agree with tim here, that the benefit seems quite low.

Really? I think this is a HUGE improvement. Yes, it is less imperative on a smaller dialog like this but your last comment "how this stretches to a pattern across core/contrib." is where the real benefit will be as we get into longer and more complex forms.

@Tim.Plunkett

Because this will usually be opened in a modal, splitting the available width between two columns leads to it being rather squished.
In addition, we lose the "summary" for each tab, like what roles or content types it is visible for.

I think the "squishing" issue has to do with the default width of the modal being too narrow.
On the "summary" question I would add them back. I definitely think they add value.

tkoleary’s picture

@Bojhan, @tim.plunkett

Here's a rough estimation of how it should ultimately look with Seven style patches applied. Hacked together with Chrome inspector but all possible with current markup.

IMHO looks much better than before taken in totality.

image

tkoleary’s picture

@Bojhan, @tim.plunkett

Angie commented that the machine name still wraps. This can be solved by increasing the size of the modal (currently 700px), or by setting a min width on machine name and making it display inline block so the whole thing kicks down below the title field at his width.

Perhaps both.

tim.plunkett’s picture

My screenshot of the new modal bumped the width up to 800px.
What is the largest we can safely make a modal?

The "summary" JS isn't natively compatible with the details elements in the style, I'll have to play with it more.

tim.plunkett’s picture

FileSize
756 bytes
4.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es). View

Still at 800px, I adjusted the proportions, and added the inline-block for the machine name.

The padding on the top and right of the modal is really out of normal control of the content, so I'm not using any crazy tricks to make the right column flush just yet.

Wim Leers’s picture

Screenshot of #7:

Screen Shot 2013-09-06 at 16.30.56.png

tkoleary’s picture

@Tim Plunkett

The padding on the top and right of the modal is really out of normal control of the content, so I'm not using any crazy tricks to make the right column flush just yet.

Nothing "crazy" is needed :). Just:

.block-secondary, .block-list-secondary {
  position: absolute;
  right: 0;
  top: 0;
  border: 0px;
  border-left: 1px solid #999;
  height: 100%;
  overflow: scroll;
}
tkoleary’s picture

Above code produces this:

image

Note: The scroll on the column is so that you don't scroll the main form out of view.

tkoleary’s picture

Sorry,

The selector was too specific. Should be:

ui-dialog .block-secondary  {
  position: absolute;
  right: 0;
  top: 0;
  border: 0px;
  border-left: 1px solid #999;
  height: 100%;
  overflow: scroll;
  background: #eee;
}
Jeff Burnz’s picture

Just a comment, which may or may not be out of scope here, because I can't scroll the modal it can be obscured by both toolbar and the fold (current design in head), the new design proposed here would not escape this problem - if I can't scroll it away (like I can with Views) then I might not see the submit, or a field, aka its hiding behind the toolbar/tray etc.

Users need control over the scroll, like Views allows, so if stuff don't fit vertically I can just scroll to see it all. I dont wanna be zooming in/out to submit etc.

FWIW I like the current design in head more than the two columns proposed here, the second col is cramping up stuff and vertical tabs are so common in core as a UI pattern, they make sense imo.

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

The padding on the top and right of the modal is really out of normal control of the content, so I'm not using any crazy tricks to make the right column flush just yet.

I have to agree with Tim here. I've very reluctant to set a column like this to position absolute in a one-off way. Absolute position is meant to place an element in a very specific location, not to defeat padding or get around default styling. If we want to have elements like this flush with the edges of its container, then we will need to flip the padding on the container to margins on the contents. That's a very big change. This is also something a theme could do in order to achieve a very specific visual style. But for core, it has implications beyond this form. If we absolutely position just this one piece, then it will no longer flow and might potentially break responsive layouts of the form elements.

The major issues I'm having with this patch all have to do with the persnickety, naughty and opinionated behavior of the Drupal dialog. It jumps around and won't resize well.

We have an issue to deal with collisions with displacing elements like the Toolbar:
#2067263: Drupal dialog placement must take displacing viewport elements, like the Toolbar, into account when calculating placement

Z-indexing issue:
#2072037: Drupal dialog modal background z-index is set too low to reliably occlude core UI components

And resizing issue:
#2083703: Resizable Drupal dialog refuses to resize if given a specific width option

These are all tagged with modal placement
https://drupal.org/project/issues/search/drupal?issue_tags=modal%20place...

The code in this patch involves simple template updates that leverage core systems. I don't want to go solving core problems here or hacking in overrides. The more pain we expose through using these systems, the more pressure we'll feel to address them during UX polish.

jessebeach’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, didn't mean to RTBC. This design needs more thought. It requires a fairly wide screen to view the form. We're not going to be able to take advantage of Media Queries here because the viewport size is not a true measure of the available space. The dialog width is really the triggering bounding container. And the dialog width depends largely on the dialog contents.

If we really need this much interactive stuff in a form, then the form should be a page, not a dialog. This issue needs more design work.

tkoleary’s picture

@jessebeach

Thanks for the detail. That certainly puts a different perspective on it.

Having said that let me just clarify my thinking on this. First off the absolute positioning is a red herring. That was merely the most expedient method to get the desired appearance, I could have as easily used negative margin but that seemed even more hacky.

Fundamentally, the face that padding effects two columns is the problem. The purpose of padding (at least I as I understand it from a design perspective) is to provide space between content (text, images etc.) and it's container. The error here, it seems to me, is that we have padding on a container that contains two columns. Obviously the content on the left requires padding, but not it's parent, in this case #drupa-modal. All that is needed here is to remove the padding on #Drupal-modal, of course the implication of that is that anything that goes into #drupal-modal would require it's own padding. But if we agree that modals should be allowed to have two columns then the default should be at least one column with padding.

That seems far less hacky to me, and shouldn't adversely effect any of the above issues.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: +Needs usability review, +Needs design review

Not much I can do here until there is consensus.

tkoleary’s picture

@Tim Plunkett

Jesse and I discussed. This is not on the must have list so it's after whatever's on your plate anyway. Let's discuss in Prague.

Bojhan’s picture

Version: 8.x-dev » 9.x-dev
Issue tags: -Needs usability review

Nothing to review here, it seems like its clear there are many issues from both a UX as a technical side.

catch’s picture

Version: 9.x-dev » 8.1.x-dev
catch’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update
tkoleary’s picture

Now that we have the two column node edit form it makes sense to use the same pattern in other places as well, especially modals where the experience should be as compact and immediate as possible. The place blocks modal is a good place to start since this will be one that site builders will use quite often.

tkoleary’s picture

FileSize
34.39 KB
37.14 KB
38.08 KB

Updated issue summary

tkoleary’s picture

Updated summary

tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
FileSize
28.99 KB
30.36 KB
29.67 KB
tkoleary’s picture

Issue summary: View changes
FileSize
36.36 KB
37.26 KB
34.19 KB
37.26 KB
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
FileSize
247.94 KB
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

@catch

re#20 see the motivation section I added to the summary.

This seems to me to qualify. I think it will be a big improvement

YesCT’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update +Usability

adding usability tag. updating the beta allowed and a typo in summary.

tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
jibran’s picture

YesCT’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review

so, evaluating for beta seems to disagree with the version. changing the issue meta data.

note, the review needed here is of the new design.

Bojhan’s picture

If we reuse a pattern, please re-use the pattern. Don't add your own styling.

tkoleary’s picture

@bojhan

Part of the idea here is to remove an anti-pattern to the style guide and to do it everywhere. See referenced issue.

Bojhan’s picture

@tkoleary Sure, can we move that to that issue though - we don't have to sideline it in here.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tkoleary’s picture

Version: 8.1.x-dev » 8.2.x-dev

Given that place block has landed, this would be a good enhancement.

Artusamak’s picture

Assigned: Unassigned » Artusamak

I will try to give a shot at this.

Artusamak’s picture

There we go. I followed the node implementation where we override the output in seven to let others themes manipulate vertical tabs if they want to.

This change now needs some love from a frontend person. Please note that we have vertical tabs here which means that if a value is selected within the tab, the tab title changes. This information is lost on the screenshots previously attached to this issue.
Here is a preview of the default state and with a value selected.
Visibility settings folded
Visibility settings open

Status: Needs review » Needs work

The last submitted patch, 46: core-blocks_two_cols-2079037-46.patch, failed testing.

Artusamak’s picture

Do not generate the patch files too fast while you are in a conference, otherwise you forget to commit the tpl and css files.

Artusamak’s picture

Status: Needs work » Needs review

Send for testing.

malavya’s picture

  1. +++ b/core/themes/seven/css/layout/block-add.css
    @@ -0,0 +1,48 @@
    \ No newline at end of file
    

    Small nitpick. Expected new line at EOF

  2. +++ b/core/themes/seven/seven.theme
    @@ -186,3 +186,18 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
    + * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\node\NodeForm.
    

    Nodeform?

tim.plunkett’s picture

I think this issue should be blocked on #2061863: Make two column node CSS reusable

Status: Needs review » Needs work

The last submitted patch, 48: core-blocks_two_cols-2079037-47.patch, failed testing.

tkoleary’s picture

@tim.plunkett

I think that issue was resolved. At least as far as making the layout css re-usable. But that doesn't address the styling.

@artusamak

There are two things you could do here:

1. "borrow" the CSS class .entity-meta from the node form. That would involve wrapping the H3 and all of the details elements in a div with the class .entity-meta and then wrapping just the H3 in a div with the class .entity-meta__header. Both of those also need: js-form-wrapper form-wrapper.

or

2. Duplicate those classes into block module and re-name them .block-meta, and .block-meta__header, with the same markup as above.

Either of those should produce styling that matches the node form.

If I understand Tim correctly, the unresolved task is to make a class something like just ".meta" that lives in neither node or block module. I'm not sure where or how that would be done.

Tim, can we not get away with option #1?

tim.plunkett’s picture

.entity-meta is the styling, not the layout. And that is properly decoupled, but only present in Seven.

The layout code for the node form is at the top of core/modules/node/css/node.module.css, the classes are:

.layout-region-node-main
.layout-region-node-secondary
.layout-region-node-footer

Those are what is in scope for #2061863: Make two column node CSS reusable.

tkoleary’s picture

.entity-meta is the styling, not the layout. And that is properly decoupled, but only present in Seven.

Ah, I see. But is it Seven that adds the .entity-meta class to the markup? If not where does that come from?

tim.plunkett’s picture

\Drupal\node\NodeForm::form() adds the class entity-meta. Of the core themes, only Seven provides CSS for that class. But it's added globally.
We should feel free to use entity-meta anywhere in core.

It's the layout stuff that this will be blocked on.

tkoleary’s picture

I see, but for the styling we could do something like:

  <div class="layout-region layout-region-block-secondary">
  	<div class="entity-meta">
	    <div class="entity-meta__header">
	    	<h3>{{ 'Visibility'|t }}</h3>
	    </div>
	    {{ form.visibility.title }}
	    {{ form.visibility }}
	</div>
  </div>

And those classes will be available?

tim.plunkett’s picture

layout-region-block-secondary doesn't exist. To do so we'd need to duplicate ALL of the node layout CSS from node.module.css.

Ideally the other issue would just add layout-region-entity-[main|secondary|footer] and both block and node would use that.

malavya’s picture

@Artusamak

For the last patch you submitted, apart from the above mentioned points found a few issues.

1. The selected values in the vertical tabs don't show up in the tab title.
no%20selected%20option%20in%20tab%20title.png

2. On saving the block the page redirects to the block library page.

EclipseGc’s picture

Could we discuss removing the visibility conditions from the block configuration screen completely? This would be much better in a separate form.

Eclipse

tim.plunkett’s picture

yoroy’s picture

@EclipseGc interesting, but *why* would it be much better in a separate form?

EclipseGc’s picture

@yoroy

Currently, we have to validate all those form elements even though 90% of the time the user isn't interacting with any of them. In contrib (especially) this can lead to situations where we're not certain if the user interacted with a block's visibility conditions or not, which can lead to false positives on a given visibility condition which might mean your block doesn't show up, even though you didn't do anything to hide it.

Moving this to a separate form is a great way to prevent this sort of bad behavior for the 90% case right away, and re-engineering that form so that we explicitly add conditions we want applied (instead of having all of them applied all the time) would take care of the other 10% and could be done in a follow up.

I've been asking us to seriously consider doing this since at least Amsterdam. It would be a big big improvement. Also, I'll point out that the two column approach falls apart as soon as we have ctools or rules installed in a code base since both of them provide a whole host of new conditions which can quickly pollute this UI.

Eclipse

tkoleary’s picture

@eclipsegc

I think this is a good idea, provided the visibility options form is conditional and not a required second step, eg. two submit options: a) Save and close, b) save and configure visibility.

In that case choosing "a" is an explicit statement "do nothing to the visibility settings"

EclipseGc’s picture

100% agree with that. I'd also add "visibility setting" to the block list/reorder page on the individual block options, so "Edit", "Visibility Settings", "Delete.

Eclipse

Bojhan’s picture

I don't want to break any bubbles here, but historically our whole "save" and "Save and do another thing" has not worked well in testing of either the Field UI as the Views UI - peolpe continuously click the wrong one. It was a compromise a long time ago.

We can use tabs here, we had that in the initial style guide. But that would be a big diversion from the initial issue opener :)

EclipseGc’s picture

Yeah, so I don't care about the "Save and" process. We can do that or we can skip that. But we should NOT do this two column approach here for all the reasons I outlined in #63

Eclipse

Jeff Burnz’s picture

@eclipsegc so if there were two tabs we could split the form and run seperate validation and submit etc, I assume Bojhan is talking about this with regards to tabs, but applied to modals: https://groups.drupal.org/node/283223#Overlay - it has tabs.

tkoleary’s picture

@eclipsegc

While it *may* be the case that there should be a separate form, the current reality is that there is not a separate form, and the purpose of this issue is *only* to extend a visual layout paradigm that we have already introduced into core with the entity meta column at /node-edit.

If you want to open a separate issue to put visibility settings into a different form I'm 100% behind that effort, but let's confine the scope of this issue to layout, since there well may be many other types of "meta-settings " a block may want to specify for this column that are not visibility settings.

EclipseGc’s picture

@tkoleary

That's fine, and I think we generally agree. My primary point in commenting here is essentially "don't create yet another thing to (un)do in an already difficult UI". The proposal isn't a fix so much as it's kind of white washing a tomb (so to speak). It's prettier, and nicer, but it's also totally non-essentially and makes undoing the actual problem more difficult. So if we were to work on removing visibility conditions from this UI, but the side bar is for the visibility conditions, then if this issue were already committed, we'd have to essentially undo it. Is this sensible? or am I missing something?

Eclipse

tkoleary’s picture

@eclipsegc

Woah! You're going all old testament on me. :)

But yes, I think you are missing the point that there certainly will be other things that will go in the right column that are not visibility settings. The idea of using a two column structure for primary and secondary information was not meant to be unique to the node form but should be a pattern that any form can use.

EclipseGc’s picture

Awesome, so what goes there in block configuration if visibility were not on this form?

Eclipse

PS: (That's New Testament, but still definitely Biblical :-D)

tkoleary’s picture

Awesome, so what goes there in block configuration if visibility were not on this form?

In core, nothing, until a module (or core) adds something.

EclipseGc’s picture

So what would that look like? (serious question) Are we just imagining a two tone kind of visual with an empty second column?

Eclipse

tkoleary’s picture

I imagine it would behave like an empty region and just collapse.

EclipseGc’s picture

Ok, so visually no real change from what we see today, but the potential for more would be built in?

Eclipse