[Notifications] Add padding to notifications
ClosedPublic

Authored by rooty on Jan 5 2019, 12:31 AM.

Details

Summary

This patch aims to add padding to the notification widget and fully remedy the issues that arise once it is applied.

Test Plan

Before:

After:

(More padding, and margins as even as can be)

Diff Detail

Repository
R120 Plasma Workspace
Branch
notification-padding (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6655
Build 6673: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
rooty marked an inline comment as done.Jan 5 2019, 4:54 AM

A bit more padding is good!

  1. Making this pixel-perfect with every combination of font and font size is going to be impossible sadly. With that in mind, the most elegant design is one that makes this not a problem, because it doesn't have to be pixel-perfect and all the baselines are at least aligned.

Yeah...

  1. No matter what you pick here, it will look wrong in some cases. :( If you anchor the icon to the top left, it will look bad the moment there's a second line of text. But if you vertically center it, then it looks bad anytime the notification is tall. Judgment call I think, but personally I'm in favor of anchoring it to the top left.

Top left sounds good. I'm on the fence to be honest, but I thought to myself hey this is a problem for another diff anyway so why not take care of the padding first 😆

applets/notifications/package/contents/ui/NotificationItem.qml
175

Is there any way around this? Because if it's not 0.75 then it's shifted too far down (uneven padding)

davidedmundson added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
175

Is there any way around this? Because if it's not 0.75 then it's shifted too far down (uneven padding)

If that's the case, adjust the other side, even adjust the window if you need to.

rooty added a comment.EditedJan 6 2019, 9:05 AM

@davidedmundson Is there anything I can do to the code to get the icons to scale properly (proportionately)? This is a screenshot of master (not this patch):

I've tried multiplying units.iconSizes.large by the scaling factor, and that works, but that's as ballpark a solution as they come, and hardly permanent...

applets/notifications/package/contents/ui/NotificationItem.qml
175

Nate's right though, I should abandon the idea of totally even padding because it'll never hold up as you change fonts or scaling... then again it doesn't have to be perfect, right? Just close enough?

How are you scaling plasma? Are you setting PLASMA_USE_QT_SCALING=1 to make sure it's really actually using the scale factor you set in the KScreen KCM?

Technically Plasma doesn't support fractional scale factors yet (see https://bugs.kde.org/show_bug.cgi?id=356446) so if the only bugs you have occur there, maybe we can live with that?

How are you scaling plasma? Are you setting PLASMA_USE_QT_SCALING=1 to make sure it's really actually using the scale factor you set in the KScreen KCM?

You don't need to set that. Plasma does it's own thing with fonts, fonts will be changed regardless.

Technically Plasma doesn't support fractional scale factors yet

It kinda does because it does it's own thing with the fonts...unless you set that env var above.

rooty added a comment.Jan 7 2019, 11:50 PM

Hey guys, I feel silly asking this but I'm using a different computer right now - how do I get this computer's arcanist to lock onto and update this diff (instead of creating an entirely new diff...)?

Hey guys, I feel silly asking this but I'm using a different computer right now - how do I get this computer's arcanist to lock onto and update this diff (instead of creating an entirely new diff...)?

arc patch D17975 # Downloads the patch to a local branch
<change stuff>
arc diff --update D17975 # forces arc to update this diff instead of creating a new one, on the off chance that it would try to
rooty updated this revision to Diff 48943.Jan 8 2019, 1:29 AM

Use integers/halves and line counts

rooty added a comment.Jan 8 2019, 1:31 AM

I removed "0.75 * units.smallSpacing" because it's not an integer - the result is that the margins are slightly uneven, but considering they weren't ever entirely even, and the difference between the margins gets more pronounced with increasing font size (and scaling factor), but isn't noticeable, maybe it's a nonissue?

That being said, I've also added a line that raises the notification contents by units.smallSpacing if there is more than one line of text, to make the margins more even.
Before:


After:

Could you update the images in the Summary section instead of posting new ones in comments? The visual history of the patch is not really important to preserve, and having the images on top always be current makes reviewing it much easier. Thanks!

rooty added a comment.Jan 8 2019, 1:34 AM

Could you update the images in the Summary section instead of posting new ones in comments? The visual history of the patch is not really important to preserve, and having the images on top always be current makes reviewing it much easier. Thanks!

haha sure thing, can i just do the before and after then? no more stream of consciousness?

Before and after would be perfect, yes!

rooty edited the test plan for this revision. (Show Details)Jan 8 2019, 1:43 AM
filipf added a subscriber: filipf.Jan 8 2019, 12:41 PM

I have some concerns about this. Mostly it's about visual consistency and notifications having different padding than other plasmoids. Notice how there's currently more or less equal distance to the left of Audio Volume and to the left of the album cover icon?

Wouldn't it be better to somehow add padding globally?

Plus, some desktop themes already add padding, which would make their notifications look bad. Can you test with some other desktop themes?

abetts added a subscriber: abetts.Jan 8 2019, 3:36 PM

I have some concerns about this. Mostly it's about visual consistency and notifications having different padding than other plasmoids. Notice how there's currently more or less equal distance to the left of Audio Volume and to the left of the album cover icon?

Wouldn't it be better to somehow add padding globally?

Plus, some desktop themes already add padding, which would make their notifications look bad. Can you test with some other desktop themes?

I would be in favor of more consistency wherever possible. However if we can't get that, I would still vote for having these changes applied to notifications rather than not have them because of inconsistencies elsewhere.

I would be in favor of more consistency wherever possible. However if we can't get that, I would still vote for having these changes applied to notifications rather than not have them because of inconsistencies elsewhere.

+1. FWIW, adding padding globally is probably not possible because of how specific layouts are constructed. We'd need to do it on an individual basis.

This is no longer WIP, right?

rooty added a comment.Jan 8 2019, 10:49 PM

This is no longer WIP, right?

Oh so I can rename it? Sure!

rooty retitled this revision from [WIP, Notifications] Add padding to notifications to [Notifications] Add padding to notifications.Jan 8 2019, 10:49 PM
davidedmundson added inline comments.Jan 8 2019, 11:05 PM
applets/notifications/package/contents/ui/NotificationItem.qml
33

Much better.

Only part I don't understand, then ship it! from me.

Assuming there's no icon

For multiline

  • 1 small space above 1 small space under.

For one line we effectively get:

  • 0 margin on top, 0.5 under

Why the 0.5? Surely that would be inconsistent?

(In all your examples the icon is bigger so we don't see it)

filipf added a comment.EditedJan 8 2019, 11:18 PM

This is what I get when I test on 2 of my machines:

Codezela added a subscriber: Codezela.EditedJan 8 2019, 11:24 PM

why every notification width is too wide
why its not resize depends on the content on it with some little padding or anything else
this empty space make the card bigger and ugler

rooty added a comment.Jan 8 2019, 11:31 PM

Wouldn't it be better to somehow add padding globally?

Not really feasible because you don't necessarily want padding everywhere (for example, Adapta does this and i think it looks unsightly)


Adding padding everywhere might involve modifying every heading and every icon, that's just not what we want to do (you don't, for instance, want padding around system settings sidebar icons which might be a side effect of what you're suggesting)

If you limit yourself to only text/headings, then adding padding may make your GUI elements look disproportionate (the text gets padding, the icon doesn't and gets pushed up and to the left (provided the theme doesn't provide you with padding), which is what happened in some of my mockups)

Breaking other desktop themes is unfortunate, but our primary responsibility is in my opinion to Breeze - maybe we can find a way to modify Breeze to add more padding/cushioning, but then two problems arise:

(1) This would prevent the text from being shifted upward with a line count greater than 1 and cause even greater inconsistency in the margins (they would become very asymmetrical) - you could work around this but these workaround are probably going to be very inelegant
(1) These modifications would have to be implemented in Breeze Dark, Breeze LIght and possibly even Breath (the main theme Manjaro Linux uses), and any other theme that wants to follow suit
(3) This would also open up a couple of other very interesting questions - should other Plasma elements be modified, or should the widgets that call for them be modified? Should we rely on desktop themes to add padding to "Audio Volume" or "Status & Notifications"?

rooty added a comment.Jan 8 2019, 11:32 PM

why every notification width is too wide
why its not resize depends on the content on it with some little padding

what do you mean? the width of the entire notification?

why every notification width is too wide
why its not resize depends on the content on it with some little padding

what do you mean? the width of the entire notification?

i mean the space between the close button and the text vertically

Breaking other desktop themes is unfortunate ...

It's a bit more than unfortunate. Similar things have happened recently e.g. with the new media player icons and with the network widget. This causes tangible annoyance among the user base. The best solution for Breeze should be the priority, yes, but not if there are solutions that work well for all parties involved. I'd much rather if there was some padding added in Breeze and then you only fix the wrong spacing to right of the notification. Even if that wouldn't work, I don't get the sense we've fully explored other solutions yet.

So what's going on here? Other themes add more padding? If so, I don't see how that's something we can possibly account for, and that shouldn't affect our ability to improve our own stuff.

rooty added a comment.Jan 8 2019, 11:54 PM

I'd much rather if there was some padding added in Breeze and then you only fix the wrong spacing to right of the notification. Even if that wouldn't work, I don't get the sense we've fully explored other solutions yet.

There are other problems though besides the right margin of the icon. The heading can't be lifted to stop the icon from towering over it because the theme provides its own padding and, in essence, locks it into place.

In other words, the padding here varies depending on the amount of notification content, which I don't think is something a theme can account for.

Modifying Breeze to add padding would restrict its ability to manipulate the notification content fully.

So what's going on here? Other themes add more padding? If so, I don't see how that's something we can possibly account for, and that shouldn't affect our ability to improve our own stuff.

We may be able to account for it, but our hands are tied here I'm afraid because of the technical difficulties of making the Breeze theme doing what I want the notifications to do in this patch (vary padding by notification content).

applets/notifications/package/contents/ui/NotificationItem.qml
33

You're right! I never tested it with just the heading, There's an extra 0.5 * units.smallSpacing of padding there. But I needed that 0.5 units to provide even padding to notifications that are "Heading + One line of description".

Is there any way I can put in a condition that if it's just a heading that it not put that 0.5 * units.smallSpacing there?

So what's going on here? Other themes add more padding? If so, I don't see how that's something we can possibly account for, and that shouldn't affect our ability to improve our own stuff.

Yes, as you can see in the pics it doesn't just add extra padding but may also result in worse symmetry before.

I'm simply saying we try to investigate if there might be a solution that improves our stuff, but doesn't regress other stuff.

People do use other themes and they do notice when things get messed up. Example of the last time this happened:

https://www.reddit.com/r/kde/comments/8rj3k7/wrong_media_player_icon_after_513_update/
https://www.reddit.com/r/ManjaroLinux/comments/90drav/media_player_tray_icon_missing_kde/
https://www.reddit.com/r/kde/comments/93thby/issues_with_the_new_media_player_icons/
https://forum.manjaro.org/t/manjaro-18-kde-media-player-no-panel-icon-using-breath-theme-set/64010
https://forum.manjaro.org/t/kde-breath-theme-missing-icons/51644
https://forum.manjaro.org/t/kde-media-player-indicator-slot-empty/64199
https://forum.manjaro.org/t/manjaro-18-kde-media-player-no-panel-icon-using-breath-theme-set/64010
https://forum.manjaro.org/t/smplayer-missing-tray-icon-in-kde-when-playing-movie/58021

I wouldn't want the attitude of "breaking other themes is unfortunate, but.." becoming a pattern because openness to customization is our main selling point.

rooty added a comment.Jan 9 2019, 12:10 AM

@davidedmundson I've tried using

implicitHeight: Math.max(appIconItem.valid || imageItem.nativeWidth > 0 ? units.iconSizes.large : 0, (mainLayout.height + ((bodyText.lineCount > 1) ? 0.5 : ((bodyText.lineCount == 0) ? 0 : 2) * units.smallSpacing))) // Add units.smallSpacing to correct for the topMargin of mainLayout according to number of lines

(I know, it looks ugly...) but it doesn't seem to do anything

rooty added a comment.EditedJan 9 2019, 12:21 AM

I wouldn't want the attitude of "breaking other themes is unfortunate, but.." becoming a pattern because openness to customization is our main selling point.

You've made your point. But allow me to flesh it out - it seems we've reached an impasse; in simple terms, you're either going to maintain the themes and restrict Plasma's growth or you're going to remedy the problems this widget has exhibited and step on a few toes.

You can, in fact, modify Breeze to add padding, sure. But if you do so, you can't move the notification text up (or around, if you should want to). I'm kind of belaboring the point here - that means you can't get padding that stretches and contracts naturally depending on how much notification content there is. (I never wanted just padding.)

You can also manipulate the notification icon to achieve the same effect that I achieved by lowering the value of topMargin of the notification heading. However even then, padding that's arrived at through theme manipulation is still always going to remain the same.

I am open to suggestions on how to resolve this deadlock.

@filipf
The media player situation was very different. This won't break anything.

@rooty

((bodyText.lineCount > 1) ? 0.5 : ((bodyText.lineCount == 0) ? 0 : 2) * units.smallSpacing)))

There's a logic error. I'll rewrite it so you can see it.

if (lineCount > 1) {
    return 0.5;
} else {
   if (lineCount == 0) {   
      return 0;
   } else {
     return 2;
   }
}

BTW, if you want a neater solution: (though consider this optional)

At the moment you're moving mainLayout about and then doing maths on the window height to make sure we get the same padding underneath.

Instead, use Layout.topMargin on the titleBar and Layout.bottomMargin on the lowerPart.

Then QtLayouts can do all the hard work, and you're spreading this logic about to the relevant places next to where you want the padding.

(you'll have to bump "import QtQuick.Layouts 1.1" to 1.2)

rooty added a comment.EditedJan 9 2019, 2:28 AM

BTW, if you want a neater solution: (though consider this optional)
Instead, use Layout.topMargin on the titleBar and Layout.bottomMargin on the lowerPart.

I love it. It's just that the icon doesn't seem to get the message :/ That's why I've been manipulating the height this entire time. I'd like to implement this but I don't know how to add padding to IconItem... (I can't think of anything to anchor it to - anchoring it to parent.bottom and adding a margin stretches the icon out / distorts it)

This works, but it looks messy:

implicitHeight: Math.max(appIconItem.valid || imageItem.nativeWidth > 0 ? units.iconSizes.large : 0, (mainLayout.height + ((bodyText.lineCount > 1) ? 1 : ((bottomPart.height == 0) ? 0 : 2) * units.smallSpacing))) // Add units.smallSpacing to correct for the topMargin of mainLayout according to number of lines
broulik added a subscriber: broulik.Jan 9 2019, 8:55 AM

We explicitly removed the padding in Plasma 5.9 to reduce popup sizes, this effectively reverts this decision…

filipf added a comment.EditedJan 9 2019, 11:32 AM

FWIW if I backport the spacings and margins prior to D3560 it works great for me. The symmetry around the icon is better than with this diff. (only NotificationItem.qml is modified)

This diff:

Old:

rooty added a comment.Jan 9 2019, 11:32 AM

We explicitly removed the padding in Plasma 5.9 to reduce popup sizes, this effectively reverts this decision…

Sort of. This is basically what they looked like:


And the result of D3560 were a slew of custom themes that added the padding back. The main justification was:

Our notification popups are huge compared to other platforms

and that's just not the case. At least not anymore:


I grant that notifications without padding do save room. But it's really not that much room we're saving? Especially considering that a notification is something a user should be paying attention to.
As far as text on the buttons is concerned, I'm confident we can make it work and that the labels need not be too short or too long, or in other words, that this is an issue completely separate from the padding issue.

rooty added a comment.EditedJan 9 2019, 11:39 AM

FWIW if I backport the spacings and margins prior to D3560 it works great for me. The symmetry around the icon is better than with this diff. (only NotificationItem.qml is modified)

This diff:

Old:

Try rolling back NotificationPopup.qml too. They should look very similar to the ones in this patch. (The idea is that the notification in the second screenshot has padding only because arc-kde provides it.)

filipf added a comment.EditedJan 9 2019, 11:48 AM

FWIW if I backport the spacings and margins prior to D3560 it works great for me. The symmetry around the icon is better than with this diff. (only NotificationItem.qml is modified)

This diff:

Old:

Try rolling back NotificationPopup.qml too. They should look very similar to the ones in this patch. (The idea is that the notification in the second screenshot has padding only because arc-kde provides it.)

Still better with the old values:

EDIT: With defaults yours look better, but basically because they're compensating for too much space below the icon

rooty added a comment.EditedJan 9 2019, 11:48 AM

@broulik If the consensus should end up being that the notifications should stay "tight" (no padding), I'd very much prefer to add padding to the left and right of the icon (it's really stuck-on right now), and to push the notification icon down a bit if there's a bodyText.lineCount > 1 to prevent the icon from towering over the heading.
For example,

and

But I very much prefer the notifications with padding all around :D

rooty updated this revision to Diff 49110.Jan 9 2019, 9:13 PM

Change certain integers to decimals for more even padding; fix padding in heading-only notifications

davidedmundson accepted this revision.Jan 9 2019, 11:20 PM
This revision is now accepted and ready to land.Jan 9 2019, 11:20 PM
rooty added a comment.EditedJan 10 2019, 1:12 AM

Hey @davidedmundson I know I'm a little late but should I use your suggestion instead? I just figured out a couple of minutes ago how to do it using Layout.bottomMargin (fixed the padding beneath the icon) :D The end result is the same though.

Thank you for accepting the patch!

The end result is the same though.

If the code's neater, lets see it.

rooty updated this revision to Diff 49131.Jan 10 2019, 2:07 AM

Use margin in bottomPart instead

rooty updated this revision to Diff 49132.Jan 10 2019, 2:07 AM

Remove unnecessary comments

rooty added a comment.EditedJan 10 2019, 2:12 AM

There is one small problem with almost every version I've submitted so far. It manifests if a font other than Noto Sans is used.


Provided there's an icon (usually a square, 48 x 48 px, album art for example) and one line of heading + one line of description, the padding above the icon and below the icon is the same (7 px with 1x scaling) if Noto Sans is used as the default font, which is basically perfect.

However, if another font is used, the amount of padding below the icon changes (Cabin - 5 px, Lato - 3 px etc.). It's because I've been using bottomPart or the implicitHeight to make the notification "taller" so as to give the icon padding.

Is there a way to give the icon itself padding? I've been using anchors but there's nothing I can think of to anchor it to? If I anchor it to the notification text in any way, I can add a margin, sure, but it gets stretched out beyond belief if there's too much text.

rooty added a comment.EditedJan 10 2019, 3:55 AM

Is there a way to give the icon itself padding? I've been using anchors but there's nothing I can think of to anchor it to? If I anchor it to the notification text in any way, I can add a margin, sure, but it gets stretched out beyond belief if there's too much text.

After playing around with the code (I'm still very new to this), I came up with this:

PlasmaCore.IconItem {
  id: appIconItem

  width: summaryLabel.height * 2 - 1.5 * units.smallSpacing
  height: (bodyText.lineCount > 1) ? (summaryLabel.height * 2 - 1.5 * units.smallSpacing) : (summaryLabel.height + bodyTextScrollArea.height + 0.5 * units.smallSpacing)

I've actually edited this post quite a number of times but I've managed to come up with this because it's the only thing that fends off the incessant binding loops that result from defining the icon width using the width of the bottom part (which is, in turn, I presume defined by the icon width etc.).
This isn't supposed to produce a perfect square and yet it often does?
And it works without binding loops (thank goodness!).
But it still seems a little crude?

ngraham requested changes to this revision.Jan 10 2019, 7:59 PM

If I use notify-send to create a simple notification, it's unfortunately a regression compared to what we have right now. Checkout the uneven top and bottom padding on the icon.

Status quo:

With this patch:

This revision now requires changes to proceed.Jan 10 2019, 7:59 PM
rooty added a comment.EditedJan 10 2019, 8:49 PM

With this patch:

TLDR: Yup, but that's only the tip of the iceberg (of the problems that plague both master and this patch). Suffice it to say, neither master nor this patch scales properly with regard to font size or scaling factor, and both have problems with padding.

It's the problem I described two comments ago:

However, if another font is used, the amount of padding below the icon changes (Cabin - 5 px, Lato - 3 px etc.). It's because I've been using bottomPart or the implicitHeight to make the notification "taller" so as to give the icon padding.
Is there a way to give the icon itself padding? I've been using anchors but there's nothing I can think of to anchor it to? If I anchor it to the notification text in any way, I can add a margin, sure, but it gets stretched out beyond belief if there's too much text.

I've been using fonts to illustrate the problem but your screenshots do a better job.
In addition, changing the height of the icon results in several binding loops (gross), or in an icon that's not a square (album art), or the situation in your second screenshot. Hardly ideal.

I just don't know how to add bottom padding to the notification icon itself and have the height of the notification adapt accordingly.

P.S. This did, however, shed some light on things that have been bothering me about the current master.

Status quo:

Should this happen? There's no description yet there's an empty space. The icon doesn't resize accordingly. This is what happens in master with fonts >10pt and scaling that's a non-integer that's not a multiple of 0.5 (1.1, 1.2, 1.3 etc.), resulting in padding underneath the icon (not the notification text!) and none above. For example:


The plot thickens - another screenshot of master (not this patch)

I'm assuming this second one is because of the close button. This doesn't happen with this patch but I'm assuming I merely obscured the problem rather than solved it.

I'm at a loss as to how to resolve this. The padding problems result from:

  • in master, because the icon is defined as "units.iconSizes.largeSpacing" without any regard for the user's font (or scaling factor actually, except when it's 1.5, 2, 2.5, 3)
  • in this patch, because of the units.iconSizes.largeSpacing thing and my lack of proficiency in QML and inability to give the icon padding without manipulating either its height or that of the entire notification (doing the former results in several binding loops OR an icon that's distorted in size and isn't a square... doing the latter results in the problem depicted in your screenshots)
filipf added a comment.EditedJan 10 2019, 10:25 PM

^ Yes, it's important to note that additional (and unwanted) padding underneath the icon can happen with master, while this patch can add the same above the icon. From my layman's understanding, there is nothing solid and absolute values are based on.

rooty added a comment.EditedJan 10 2019, 10:32 PM

^ Yes, it's important to note that additional (and unwanted) padding underneath the icon can happen with master, while this patch can add the same above the icon. From my layman's understanding, there is nothing solid and absolute values are based on.

There are effectively two problems here:

  • How do we get even padding around the icon itself without having to resort to manipulating the rest of the notification?
  • How do we get the icon size to increase in proportion to increasing font size (and/or scaling factor) and, once bigger/smaller, how do we get it to stay that very same size regardless of how many lines of text there are? Is this an approach we should at all consider?
rooty updated this revision to Diff 49208.Jan 10 2019, 11:50 PM

Correct padding with just icon + heading + no text, make icon padding even for 10 pt sized fonts other than Noto Sans

Is there a way to give the icon itself padding? I've been using anchors

Yes, but if you're padding the icon by moving it then you need to set the window height to

Math.max(sizeOfIcon, sizeOfText)

needs to be

Math.max(sizeOfIcon + 2*padding, sizeOfText)

in order to keep even padding.


How do we get the icon size to increase in proportion to increasing font size

See units.cpp in plasma-framework. It's complex. I don't think that's a relevant topic you need to go down

Last diff is much better in general, but Noto Sans 11pt seems to cause problems

How do we get the icon size to increase in proportion to increasing font size

See units.cpp in plasma-framework. It's complex. I don't think that's a relevant topic you need to go down

Yeah, also I'm not sure it's the right approach - icons might turn out to be HUGE...

Last diff is much better in general, but Noto Sans 11pt seems to cause problems

I have an idea how to fix that.

rooty updated this revision to Diff 49209.Jan 11 2019, 12:08 AM

Fix lower border cutoff for fonts > 10 pt

You have 4 possible states:

Icon is taller and text is one line
Icon is taller and text is multi-line
Text is taller and text is one line
Text is taller and text is multi-line

Changing font sizes is moving you between them, but that's just a side effect.


You seem to be trying different things and then testing them. From this ever lasting history that's not working.

Making margins match isn't a visual problem, it's a maths problem.

When I reviewed this last time, I didn't run it, I just did it all on paper. (genuinely, here's a photo https://home.davidedmundson.co.uk/index.php/s/W8ZtPMZNyBAXRZz)

Take a step back, do the calculations of the window height based on what things should be, and then it'll just work.

Harbormaster completed remote builds in B6939: Diff 49209.

You were right! Thank you so much, it was so much easier when I started thinking about it as a math problem rather than a design problem. I'm going to upload another diff, I hope this one stands up to scrutiny.

I apologize for the very long "implicitHeight" but it had to cover six different configurations:

  • no icon + heading only
  • no icon + heading + one line of body text
  • no icon + heading + many lines of body text
  • icon + heading only
  • icon + heading + one line of body text
  • icon + heading + many lines of body text.
rooty updated this revision to Diff 49230.EditedJan 11 2019, 12:25 PM

Use units.smallSpacing (integers) for padding consistently except when bodyText.lineCount > 1, tidy things up

davidedmundson accepted this revision.Jan 11 2019, 1:22 PM
ngraham accepted this revision.Jan 11 2019, 3:40 PM

Lovely, works perfectly now. :)

This revision is now accepted and ready to land.Jan 11 2019, 3:40 PM

@davidedmundson, does this look good to you?

@rooty if you want to re-apply for a commit account, I'll approve it.

Closed by commit R120:6a625cd61d29: [Notifications] Add padding to notifications (authored by Krešimir Čohar <kcohar@gmail.com>, committed by ngraham). · Explain WhyJan 11 2019, 4:24 PM
This revision was automatically updated to reflect the committed changes.
rooty added a comment.EditedJan 11 2019, 5:44 PM

@rooty if you want to re-apply for a commit account, I'll approve it.

Thank you you guys! I used an alias tho (Pete Cho) on identity.kde.org, but i'd like to use my real name (Krešimir Čohar), is that possible?

I believe you'll need to file a sysadmin ticket to get that changed.

rooty added a comment.Jan 11 2019, 6:43 PM

I believe you'll need to file a sysadmin ticket to get that changed.

Will do! Thanks!