Page MenuHomePhabricator

Adjust unbalanced spacing and broken scan line for elements in the filter menu
Closed, ResolvedPublic

Description

The filters menu in Recent changes page shows a list of elements that are scanned vertically. Recently the spacing became unbalanced as the result of some regression and no longer aligns with the original design (T149452).

In the example below you can notice that the "Filters" label, filter group label (e.g., "Contribution quality predictions"), and the checkboxes is different. This also makes the spacing at both sides of the checkboxes to be unbalanced.

Based on the space modules used by the style guide we may want to adjust all these these elements to be at 12px from the left edge of the panel, and for the case of checkboxes to also have 12px distance to the filter labels to their right. If the use of ems is required, we may need to approximate such distance, but let's make it be the same in all cases.

An example with the spacing adjusted to 12px is shown below (note that other alignment issues are present that will be addressed in separate tickets):

Details

Related Gerrit Patches:

Event Timeline

@Pginer-WMF Currently in the single filters there's a padding-top: 0.5em defined, resulting in


Is the top distance intended?

Change 424374 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] RCFilters: Adjust unbalanced scan line in filter menu

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

@Pginer-WMF Currently in the single filters there's a padding-top: 0.5em defined, resulting in


Is the top distance intended?

The top and bottom spacing is currently unbalanced. The idea was to have it balanced as illustrated in the original ticket (T149452) :

Note that in the original designs we were using 10px and 5px modules, but we may want to use 12px and 4px now to align with the style guide spacing modules.
I don't have a preference on how these distances are distributed through the padding, margin, or line-height of elements, just the actual distance the user perceives through their eyes.

In production:

With Volker's patch:

@Catrope Thanks for the screenshots, just one small note, the description is reversed. Above is my patch, below the production one.

@Pginer-WMF In patch resulting in above screenshot, I've been using 12px to the side of the menu items, but 6px to top and bottom as 12px got extensively tall, even 8px felt out of proportion…

@Pginer-WMF In patch resulting in above screenshot, I've been using 12px to the side of the menu items, but 6px to top and bottom as 12px got extensively tall, even 8px felt out of proportion…

I analysed the spacing in the screenshot below. I added 12x12px squares to illustrate that the spacing that is acceptable (in green) and the one that needs adjustment (red). The spacing on top seems to have some extra pixels that may be reduced to better fit (maybe another container is adding them or it is a browser glitch we can live with), but the distance between the checkbox and the label needs to be expanded.

In addition, I've noticed the "How do these work?" link appears in a new line, we should keep it in the same line as the label above.

@Pginer-WMF Well, we don't have full control over distances, as line-height also plays in this game.
Here's a screen with padding-top/-bottom: 12px, on current production state

That's feels too much in my opinion.

@Pginer-WMF Well, we don't have full control over distances, as line-height also plays in this game.

Well, technically it is possible to calculate how much space line height adds. If I have a 14px font with 18px line height, there will be 2px extra space at the top and the bottom ((18 - 14)/2). We can decide not to take it into account in our calculations, but I'd not claim it is out of our control.

Here's a screen with padding-top/-bottom: 12px, on current production state
That's feels too much in my opinion.

I'm confused by this. In the comment above, I picked your example based on 12px (left) and 6px (top and bottom), and illustrating that it works for me since the final distances match the 12px total (we can consider this as an example already of taking into account the extra space that line height adds ;-). So I'm totally fine with that approach. The only aspect that needs update from my perspective is the distance at the right of the chechbox, between the checkbox and the labels, which should be 12px and I showed in my example in red to illustrate the space inbalance at both sides (left and right) of the checkbox.

@Pginer-WMF Well, we don't have full control over distances, as line-height also plays in this game.

Well, technically it is possible to calculate how much space line height adds. If I have a 14px font with 18px line height, there will be 2px extra space at the top and the bottom ((18 - 14)/2). We can decide not to take it into account in our calculations, but I'd not claim it is out of our control.

I would claim, that it's out of our control. Please see a beautiful deep-dive into line-height and other font metrics. ;)

Here's a screen with padding-top/-bottom: 12px, on current production state
That's feels too much in my opinion.

I'm confused by this. In the comment above, I picked your example based on 12px (left) and 6px (top and bottom), and illustrating that it works for me since the final distances match the 12px total (we can consider this as an example already of taking into account the extra space that line height adds ;-). So I'm totally fine with that approach. The only aspect that needs update from my perspective is the distance at the right of the chechbox, between the checkbox and the labels, which should be 12px and I showed in my example in red to illustrate the space inbalance at both sides (left and right) of the checkbox.

Sorry, I stopped at reading your message, being irritated with having explicitly set 6px top & bottom and suddenly the measurement resulted in 12px. Note the differently rendered font family in your and my screenshot, updated example from above with 6px padding-top/-bottom, on my machine not resulting in full 12px distance.

If you're fine with above patches result (aware, that the distance between checkbox and label needs refinement, flagged this with Roan earlier this afternoon), that's great. It's for sure an improvement over status quo.

I would claim, that it's out of our control. Please see a beautiful deep-dive into line-height and other font metrics. ;)

Sorry for adding to confusion, I think these examples prove that you are right, and technology is not there yet at a point where we have full control on this. Thanks for the link.

Here's a screen with padding-top/-bottom: 12px, on current production state

If you're fine with above patches result (aware, that the distance between checkbox and label needs refinement, flagged this with Roan earlier this afternoon), that's great. It's for sure an improvement over status quo.

I'm ok with the vertical spacing adjustments you proposed. But let's not close the ticket until the checkbox horizontal spacing is fixed since aligning elements horizontally was the main purpose of the ticket.

@Pginer-WMF Happy, that we're on the same page. Clearly, the patch is still in review and will get updated with the checkbox horizontal spacing.

When that will go live?

When that will go live?

That depends on how quickly Volker amends his patch and how quickly I review it, but most likely next week's train (April 17-19).

Volker_E added a comment.EditedApr 10 2018, 6:40 PM

@Catrope The one thing, that doesn't make sense from my local testing, is “what's this” button breaking into next line. Could you have a look at your machine? The other issue would be updated within a few minutes…

That depends on how quickly Volker amends his patch and how quickly I review it, but most likely next week's train (April 17-19).

I'm adding it to the newsletter as a global news: "multiple styling fixes have been made for the filters interface".

Change 424374 merged by jenkins-bot:
[mediawiki/core@master] RCFilters: Adjust unbalanced scan line in filter menu

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

Etonkovidova closed this task as Resolved.Apr 12 2018, 11:13 PM
Etonkovidova added a subscriber: Etonkovidova.

@Volker_E this does not seem to be happening. Btw, do you have 'What's this?' and not 'How do these work?'

“what's this” button breaking into next line.

Checked the fix in betalabs (also in Windows/IE11)- all looks balanced:

Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 14 2018, 1:14 AM