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
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
Expanded and in focus
Expanded and not in focus
API changes
This makes #2061863: Make two column node CSS reusable even more important.
Remaining tasks
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 |
Comment | File | Size | Author |
---|---|---|---|
#59 | no selected option in tab title.png | 38.89 KB | imalabya |
#48 | core-blocks_two_cols-2079037-47.patch | 4.67 KB | Artusamak |
#46 | blocks-visibility-opened.png | 36.18 KB | Artusamak |
#46 | blocks-visibility-folded.png | 33.3 KB | Artusamak |
#30 | Screen Shot 2014-11-09 at 11.12.35 AM.png | 247.94 KB | tkoleary |
Comments
Comment #1
tim.plunkettHere 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:
With this patch:
Comment #2
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #3
tkoleary CreditAttribution: tkoleary commented@Bojhan
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
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.
Comment #4
tkoleary CreditAttribution: tkoleary commented@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.
Comment #5
tkoleary CreditAttribution: tkoleary commented@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.
Comment #6
tim.plunkettMy 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.
Comment #7
tim.plunkettStill 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.
Comment #8
Wim LeersScreenshot of #7:
Comment #9
tkoleary CreditAttribution: tkoleary commented@Tim Plunkett
Nothing "crazy" is needed :). Just:
Comment #10
tkoleary CreditAttribution: tkoleary commentedAbove code produces this:
Note: The scroll on the column is so that you don't scroll the main form out of view.
Comment #11
tkoleary CreditAttribution: tkoleary commentedSorry,
The selector was too specific. Should be:
Comment #12
Jeff Burnz CreditAttribution: Jeff Burnz commentedJust 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.
Comment #13
jessebeach CreditAttribution: jessebeach commentedI 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.
Comment #14
jessebeach CreditAttribution: jessebeach commentedSorry, 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.
Comment #15
tkoleary CreditAttribution: tkoleary commented@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.
Comment #16
tim.plunkettNot much I can do here until there is consensus.
Comment #17
tkoleary CreditAttribution: tkoleary commented@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.
Comment #18
Bojhan CreditAttribution: Bojhan commentedNothing to review here, it seems like its clear there are many issues from both a UX as a technical side.
Comment #19
klonosComment #20
catchComment #21
catchComment #22
tkoleary CreditAttribution: tkoleary commentedNow 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.
Comment #23
tkoleary CreditAttribution: tkoleary commentedUpdated issue summary
Comment #24
tkoleary CreditAttribution: tkoleary commentedUpdated summary
Comment #25
tkoleary CreditAttribution: tkoleary commentedComment #26
tkoleary CreditAttribution: tkoleary commentedComment #27
tkoleary CreditAttribution: tkoleary commentedComment #28
tkoleary CreditAttribution: tkoleary commentedComment #29
tkoleary CreditAttribution: tkoleary commentedComment #30
tkoleary CreditAttribution: tkoleary commentedComment #31
tkoleary CreditAttribution: tkoleary commentedComment #32
tkoleary CreditAttribution: tkoleary commentedComment #33
tkoleary CreditAttribution: tkoleary commented@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
Comment #34
YesCT CreditAttribution: YesCT commentedadding usability tag. updating the beta allowed and a typo in summary.
Comment #35
tkoleary CreditAttribution: tkoleary commentedComment #36
tkoleary CreditAttribution: tkoleary commentedComment #37
tkoleary CreditAttribution: tkoleary commentedComment #38
jibranComment #39
YesCT CreditAttribution: YesCT commentedso, evaluating for beta seems to disagree with the version. changing the issue meta data.
note, the review needed here is of the new design.
Comment #40
Bojhan CreditAttribution: Bojhan commentedIf we reuse a pattern, please re-use the pattern. Don't add your own styling.
Comment #41
tkoleary CreditAttribution: tkoleary commented@bojhan
Part of the idea here is to remove an anti-pattern to the style guide and to do it everywhere. See referenced issue.
Comment #42
Bojhan CreditAttribution: Bojhan commented@tkoleary Sure, can we move that to that issue though - we don't have to sideline it in here.
Comment #44
tkoleary CreditAttribution: tkoleary at Acquia commentedGiven that place block has landed, this would be a good enhancement.
Comment #45
ArtusamakI will try to give a shot at this.
Comment #46
ArtusamakThere 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.
Comment #48
ArtusamakDo not generate the patch files too fast while you are in a conference, otherwise you forget to commit the tpl and css files.
Comment #49
ArtusamakSend for testing.
Comment #50
imalabyaSmall nitpick. Expected new line at EOF
Nodeform?
Comment #51
tim.plunkettI think this issue should be blocked on #2061863: Make two column node CSS reusable
Comment #53
tkoleary CreditAttribution: tkoleary at Acquia commented@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?
Comment #54
tim.plunkett.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:Those are what is in scope for #2061863: Make two column node CSS reusable.
Comment #55
tkoleary CreditAttribution: tkoleary at Acquia commentedAh, I see. But is it Seven that adds the .entity-meta class to the markup? If not where does that come from?
Comment #56
tim.plunkett\Drupal\node\NodeForm::form()
adds the classentity-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.
Comment #57
tkoleary CreditAttribution: tkoleary at Acquia commentedI see, but for the styling we could do something like:
And those classes will be available?
Comment #58
tim.plunkettlayout-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.Comment #59
imalabya@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.
2. On saving the block the page redirects to the block library page.
Comment #60
EclipseGc CreditAttribution: EclipseGc at Acquia commentedCould we discuss removing the visibility conditions from the block configuration screen completely? This would be much better in a separate form.
Eclipse
Comment #61
tim.plunkettThat reminds me more of #2729413: Open "Configure Blocks" contextual links in a Modal with a simplified (quick edit) form, which could make this obsolete.
Comment #62
yoroy CreditAttribution: yoroy at Roy Scholten commented@EclipseGc interesting, but *why* would it be much better in a separate form?
Comment #63
EclipseGc CreditAttribution: EclipseGc at Acquia commented@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
Comment #64
tkoleary CreditAttribution: tkoleary at Acquia commented@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"
Comment #65
EclipseGc CreditAttribution: EclipseGc at Acquia commented100% 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
Comment #66
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI 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 :)
Comment #67
EclipseGc CreditAttribution: EclipseGc at Acquia commentedYeah, 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
Comment #68
Jeff Burnz CreditAttribution: Jeff Burnz commented@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.
Comment #69
tkoleary CreditAttribution: tkoleary at Acquia commented@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.
Comment #70
EclipseGc CreditAttribution: EclipseGc at Acquia commented@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
Comment #71
tkoleary CreditAttribution: tkoleary at Acquia commented@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.
Comment #72
EclipseGc CreditAttribution: EclipseGc at Acquia commentedAwesome, 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)
Comment #73
tkoleary CreditAttribution: tkoleary at Acquia commentedIn core, nothing, until a module (or core) adds something.
Comment #74
EclipseGc CreditAttribution: EclipseGc at Acquia commentedSo what would that look like? (serious question) Are we just imagining a two tone kind of visual with an empty second column?
Eclipse
Comment #75
tkoleary CreditAttribution: tkoleary at Acquia commentedI imagine it would behave like an empty region and just collapse.
Comment #76
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, so visually no real change from what we see today, but the potential for more would be built in?
Eclipse