Page MenuHomePhabricator

There's still too much whitespace between FieldLayouts in a FieldsetLayout
Closed, ResolvedPublic

Description

There's still too much whitespace between FieldLayouts in a FieldsetLayout in MediaWiki theme. (Note that the OOjs UI demo uses a slightly smaller font size than MediaWiki, and so there is less whitespace.)

Screenshots from T117753 and T113953:


Details

Related Gerrit Patches:

Event Timeline

matmarex created this task.Nov 4 2015, 6:16 PM
matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex added a project: OOUI.
matmarex added a subscriber: matmarex.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 4 2015, 6:16 PM

This is particularly jarring when you have a few checkbox fields one under the other.

Florian added a subscriber: Florian.Nov 4 2015, 6:49 PM

Maybe there should be a config in FieldLayout, which allows you to define the size of the bottom margin? E.g. small, which is as big as the current plain html checkboxes, and normal for the current space. It would default to normal, and HTMLCheckMatrix could define it as small, because it knows, that there are several checkboxes under each other (except for the last one maybe).

That's a different thing, we have separate widgets for a single radio/checkbox (RadioInputWidget [fairly useless by itself] and CheckboxInputWidget) and for a set of them (RadioSelectInputWidget and CheckboxMultiSelectInputWidget [doesn't exist yet, T117782]). The latter have a lot smaller spacing between the individual radios/checkboxes than the spacing between FieldLayouts.

The tasks I mentioned actually use individual CheckboxInputWidgets, and I think the spacing for this is too large.

matmarex updated the task description. (Show Details)Nov 4 2015, 7:37 PM
matmarex set Security to None.
GOIII added a subscriber: GOIII.Nov 7 2015, 11:36 PM

Currently, we seem to have the following defined for the element(s) in question...

.oo-ui-fieldLayout {
    display: block;
    margin-bottom: 1em;
}
.oo-ui-fieldLayout.oo-ui-fieldLayout-align-inline > .oo-ui-fieldLayout-body > .oo-ui-fieldLayout-field {
    padding: 0.5em 0;
}
.oo-ui-fieldLayout.oo-ui-fieldLayout-align-inline.oo-ui-labelElement > .oo-ui-fieldLayout-body > .oo-ui-labelElement-label {
    padding: 0.5em;
    padding-left: 1em;
}

It seems we should need margin-bottom: 1em; only for the last of type (assuming there is more than fieldLayout in play as shown in the screen-grabs above) and the other two class definition entries should be further refined to reflect a padding-top: 0.25em; and a padding-bottom: 0.25em; specifically.

In general, its seems odd to me to not have a container assigned and defined that can "hold" one or more of the various types of form widgets needed for any given instance at hand. That's why the 1em margin-bottom works for the group of radio widgets but not for the individual form widgets (hope that made sense).

In general, its seems odd to me to not have a container assigned and defined that can "hold" one or more of the various types of form widgets needed for any given instance at hand. That's why the 1em margin-bottom works for the group of radio widgets but not for the individual form widgets (hope that made sense).

I'm afraid that at least I did not understand what you mean. :)

Change 252497 had a related patch set uploaded (by Bartosz Dziewoński):
Reduce whitespace between FieldLayouts

https://gerrit.wikimedia.org/r/252497

In general, its seems odd to me to not have a container assigned and defined that can "hold" one or more of the various types of form widgets needed for any given instance at hand. That's why the 1em margin-bottom works for the group of radio widgets but not for the individual form widgets (hope that made sense).

I'm afraid that at least I did not understand what you mean. :)

Based on what I can gather from the examples on the Demo page, you've created examples for each type of widget and it's possible states and/or derivatives as individual stand-alone entities. Thus, each has margin and/or padding applied as if each were the only widget to appear on any given page.

In reality, most pages can not only have more than one type of widget but any number of additional types of widgets as well. So instead of having a single wrapper dealing with "spacing" for x number of widgets on any given page, we have each individual widget applying their own [cumulative] padding and spacing instead.

In short, you don't have a demo with an "universal" container where you can plug in and &/or out various types and numbers of widgets and the spacing, etc. amongst them is always the same.

Jdforrester-WMF triaged this task as Normal priority.Nov 17 2015, 1:26 AM
Jdforrester-WMF moved this task from Backlog to Doing on the OOUI board.
Jdforrester-WMF closed this task as Resolved.Nov 18 2015, 12:46 AM
Jdforrester-WMF assigned this task to matmarex.
Jdforrester-WMF edited projects, added OOjs-UI-next-release; removed Patch-For-Review.

Change 252497 merged by jenkins-bot:
Reduce whitespace between FieldLayouts

https://gerrit.wikimedia.org/r/252497

Based on what I can gather from the examples on the Demo page, you've created examples for each type of widget and it's possible states and/or derivatives as individual stand-alone entities. Thus, each has margin and/or padding applied as if each were the only widget to appear on any given page.
In reality, most pages can not only have more than one type of widget but any number of additional types of widgets as well. So instead of having a single wrapper dealing with "spacing" for x number of widgets on any given page, we have each individual widget applying their own [cumulative] padding and spacing instead.
In short, you don't have a demo with an "universal" container where you can plug in and &/or out various types and numbers of widgets and the spacing, etc. amongst them is always the same.

I honestly don't understand what you're getting at, sorry.

Individual widgets don't have any margin. The margin/padding is generated by wrapping them in a layout (generally, a FieldLayout), which always generates identical spacing regardless of widget type. These, in turn, can be wrapped in a FieldsetLayout to generate headings (sections). This is exactly what the demo page does, and what you're supposed to do in real interfaces.

GOIII added a comment.Nov 20 2015, 2:40 AM

I honestly don't understand what you're getting at, sorry.
Individual widgets don't have any margin. The margin/padding is generated by wrapping them in a layout (generally, a FieldLayout), which always generates identical spacing regardless of widget type. These, in turn, can be wrapped in a FieldsetLayout to generate headings (sections). This is exactly what the demo page does, and what you're supposed to do in real interfaces.

I'm sorry but its kind of hard to explain what I want to explain because [technically] there is little on that demo page using any sort of 'traditional' HTML "form" &/or "input" tags and syntax that I'm accustomed to seeing. Let me try again.

First let me verify the problem in simple terms:

  • There is too much space between the horizontal listing of each OOUI based check-box displayed

In other terms, using the following example to illustrate the 'problem'...


... the check-boxes labeled Move associated talk page, Leave a redirect behind, Move subpages (up to 100) and Watch source page and target page are spaced too far apart from each other. If I've [re]stated the original report correctly...

Apparently, the following is only intended for check-boxes rendering vertically, one after the other, from LTR or RTL...

.oo-ui-fieldLayout.oo-ui-fieldLayout-align-inline {
    margin-bottom: 1.25em;
}

In the attached example's case, the check-boxes are rendering horizontally, one on-top of another from top to bottom. Apparently, the intended css to "handle" this circumstance is ...

.oo-ui-fieldLayout {
    display: block;
    margin-bottom: 1em;
}

... and that gives us 1em at bottom of each check-box entity when I suspect only the last check-box found in a group (block) of one or more horizontal check boxes should be getting the 1em bottom margin (e.g. should have a :last-child definition to give last check-box some space before the "Move" button itself).

Now do you see the "flaw" I'm trying to bring to light here in the approach to vertical check-boxes & their bottom spacing as a single inline-block versus horizontal check-boxes where only the "last" check box should have the bottom spacing rather than "all" of them have now?

Jdforrester-WMF moved this task from Doing to Reviewing on the OOUI board.Nov 21 2015, 2:29 AM

Nope, these styles actually do mean to create a 1em margin after every FieldLayout. You're (and I am too) just arguing that it needs less than that. Definitely not 0, though, that would look super ugly.

GOIII added a comment.Nov 25 2015, 2:13 AM

Nope, these styles actually do mean to create a 1em margin after every FieldLayout. You're (and I am too) just arguing that it needs less than that. Definitely not 0, though, that would look super ugly.

OK then, if that is the way you want to look at it - why is the spacing between the individual radio-buttons of the RadioSelectWidgets seemingly "perfect" compared to the spacing between individual check-box widgets?

Afaict - the radio buttons do not have a margin bottom set; only a padding-top & padding-bottom of 0.25em. Am I mistaken?

You're not mistaken. There is less space between options of a RadioSelectWidgets than between FieldLayouts. This is intentional, as the options inside this widget are more closely connected than separate widgets. (Consider the case of two RadioSelectWidgets inside FieldLayouts appearing one after another.) I'm not sure why this is implemented with padding rather than margin.

Nope, these styles actually do mean to create a 1em margin after every FieldLayout. You're (and I am too) just arguing that it needs less than that. Definitely not 0, though, that would look super ugly.

Hmm, so this task isn't really solved, yet? :)