[Kickoff] Reduce the margins of KickoffItem, KickoffHighlight and use smallSpacing
ClosedPublic

Authored by ndavis on Mar 19 2019, 5:15 AM.

Details

Summary

A lot of space on the left and right of each KickoffItem was wasted, so I've reduced the margins. This improves the readability of long application titles and descriptions. I've also changed the margin units from gritUnit to smallSpacing for KickoffItem and KickoffHighlight, which slightly reduced the margins of KickoffHighlight.

Test Plan

Before

Category:


Long description:

After

Category:


Long description:

Diff Detail

Repository
R119 Plasma Desktop
Branch
KickoffItem-margins (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10481
Build 10499: arc lint + arc unit
ndavis created this revision.Mar 19 2019, 5:15 AM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 19 2019, 5:15 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ndavis requested review of this revision.Mar 19 2019, 5:15 AM
ndavis edited the test plan for this revision. (Show Details)Mar 19 2019, 5:19 AM
ndavis retitled this revision from Reduce the margins of KickoffItem to [Kickoff] Reduce the margins of KickoffItem.
ndavis edited the test plan for this revision. (Show Details)Mar 19 2019, 5:24 AM
rooty added a subscriber: rooty.Mar 19 2019, 7:10 AM

Looks a lot better, +1.

Please check whether there is subpixel placement of either the fonts or the selection rectangle with some other fonts (because gridUnit is being multiplied by a non-integer). I know it doesn't make sense but @flipwise and I have had to modify our patches too many times to count because of this.

The font that might be most sensitive to this: Lato at 11 pt

filipf added a subscriber: filipf.EditedMar 19 2019, 8:03 AM

I actually like the left margin because it contributes to visual hierarchy when headings are present:

As for long descriptions, how often would you say it occurs that you have one? From my experience, a lot of horizontal space is still just left empty in Kickoff with only minor exceptions(entries). Although I am guessing you raised the font sizes in your case so that's why the issues is a bit more prominent?

ngraham requested changes to this revision.Mar 19 2019, 11:39 AM
ngraham added a subscriber: ngraham.

+1 conceptually. Since Kickoff has its own side margins, these additional margins in the items themselves are just unnecessary. I think there's already enough indentation with category headers:

Please fix the code issues I've highlighted and then let's do more testing and get it done. :)

applets/kickoff/package/contents/ui/KickoffItem.qml
102

Don't multiply by non-integer values. units.gridUnit * 1.5 is 27, so to get roughly this value without hardcoding anything, you could use units.smallSpacing * 7 (28).

155

Ditto

This revision now requires changes to proceed.Mar 19 2019, 11:39 AM
rooty added a comment.EditedMar 19 2019, 1:57 PM

You should test this in all four orientations.
And if you retake the screenshots please arrange them as follows:

Category - before
after
Long description - before
after

It's really hard to appreciate the changes otherwise.

ndavis added a comment.EditedMar 19 2019, 4:02 PM

+1 conceptually. Since Kickoff has its own side margins, these additional margins in the items themselves are just unnecessary.

I tried to see how they would look with no margins and rather than simply filling the entire area where the item highlight is supposed to be, they extended past it. This doesn't seem like it should happen, which might indicate that something has been wrong in Kickoff for a long time, but I'm not sure why this is happening.

ndavis added a comment.EditedMar 19 2019, 4:25 PM

Another thing is the ratio between the left/right margins and the top/bottom margins varies with font size, so maybe Kickoff really does need some fixing up.

The height is units.smallSpacing * 2, plus the icon or title+subtile height, depending on which one is the highest. It should always be the needed height with a smallSpacing margin on both sides if that math is correct. This means that the fact that margins are needed to keep the icons an text within the highlight must be the cause of the inconsistency.

filipf added a subscriber: hein.EditedApr 5 2019, 3:27 PM

@hein what's your opinion on reducing the margins?

hein accepted this revision.Apr 5 2019, 3:32 PM

@hein what's your opinion on reducing the margins?

LGTM.

Thanks @hein, now we just need the requested code changes.

ndavis updated this revision to Diff 55500.Apr 5 2019, 8:54 PM

Use smallSpacing for KickoffItem and KickoffHighlight

ndavis retitled this revision from [Kickoff] Reduce the margins of KickoffItem to [Kickoff] Reduce the margins of KickoffItem, KickoffHighlight and use smallSpacing.Apr 5 2019, 9:09 PM
ndavis edited the summary of this revision. (Show Details)
ndavis edited the test plan for this revision. (Show Details)
ndavis updated this revision to Diff 55502.Apr 5 2019, 9:15 PM
ndavis marked 2 inline comments as done.
  • Remove commented out bit of old code
ndavis edited the summary of this revision. (Show Details)EditedApr 5 2019, 9:19 PM

I haven't tested this with many different fonts, but the current version of this patch works well for Noto Sans at 10, 11 and 12 pts.

filipf added a comment.Apr 5 2019, 9:22 PM

This makes Kickoff even a bit more more left-centered = looking like it's wasting horizontal space. That's why I prefer the way it was before, but I tested the patch and everything seems symmetric at least.

It seems that in the screenshots the "Small" font is set to be bigger than normal. With default font sizes Kickoff can end up looking like this (ignore the lack of few other patches):

That's a lot of unused horizontal space, while the benefits of reduced padding are only for apps with really long descriptions (which are minority) or for people using a bigger font size for "Small" text.

If everyone else is on board I won't object though.

ndavis added a comment.Apr 5 2019, 9:27 PM

This makes Kickoff even a bit more more left-centered = looking like it's wasting horizontal space. That's why I prefer the way it was before, but I tested the patch and everything seems symmetric at least.

It seems that in the screenshots the "Small" font is set to be bigger than normal. With default font sizes Kickoff can end up looking like this (ignore the lack of few other patches):

That's a lot of unused horizontal space, while the benefits of reduced padding are only for apps with really long descriptions (which are minority) or for people using a bigger font size for "Small" text.

If everyone else is on board I won't object though.

It is wasting horizontal space, no matter how the margins are done, but at least with less side margin the horizontal space can be used for long descriptions. This is a problem with the overall design of Kickoff.

Regarding the small text size, I did forget to set it back to default when doing these screenshots.

ngraham accepted this revision.Apr 5 2019, 9:27 PM

I agree that this makes it feel more left-heavy. However I still think this patch is an improvement because the artificial margins we're getting rid of don't really solve that problem either. Instead they feel arbitrary, because they are.

Ultimately I think the only way to substantially improve this situation is to do T9042. But in the short term we could probably consider making the whole thing narrower, more like Kicker. Kickoff doesn't really benefit from the increased width in its current form.

This revision is now accepted and ready to land.Apr 5 2019, 9:27 PM
This revision was automatically updated to reflect the committed changes.