[Notifications] Tweak paddings
Needs ReviewPublic

Authored by ngraham on May 10 2019, 9:50 PM.

Details

Reviewers
broulik
Group Reviewers
VDG
Summary

The new notification pop-ups are great, but they can feel a bit cramped. This patch
slightly increases the paddings around the edges.

Test Plan

Diff Detail

Repository
R120 Plasma Workspace
Branch
arcpatch-D21134
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12783
Build 12801: arc lint + arc unit
ngraham created this revision.May 10 2019, 9:50 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 10 2019, 9:50 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ngraham requested review of this revision.May 10 2019, 9:50 PM
ngraham edited the test plan for this revision. (Show Details)May 10 2019, 9:51 PM
apol added a subscriber: apol.May 10 2019, 11:50 PM

+1 I too am missing some spacing.

Not a huge fan of the dialog margins tbh. The additional row spacing inside looks fine.

Especially the top padding looks a bit off, same for the right padding to the icon.


The padding of the screenshot doesn't match the rest of the notification now

The right padding doesn't match anymore either

It also breaks when no buttons are in the title bar (that wasn't ideal before but now is even more noticeable), can be triggered when starting a copy progress and unchecking "keep progress open" in settings

Code is fine I guess.

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

Why is this not bodyLeftPadding?

264

Can't you just increase the overall spacing of the ColumnLayout rather than setting this bottomMargin all over the place?

dos added a subscriber: dos.May 12 2019, 12:51 PM

To anyone playing with those paddings: please note, that until T8177 is fixed, on X11 with hidpi screens you need to run Plasma with PLASMA_USE_QT_SCALING=1 env variable, as otherwise the font sizes are disproportionate to other UI elements.

cfeck added a subscriber: cfeck.May 12 2019, 2:05 PM
cfeck added inline comments.
applets/notifications/package/contents/ui/NotificationItem.qml
73–74

Does this take into account layouts for RTL languages?

In D21134#464005, @dos wrote:

To anyone playing with those paddings: please note, that until T8177 is fixed, on X11 with hidpi screens you need to run Plasma with PLASMA_USE_QT_SCALING=1 env variable, as otherwise the font sizes are disproportionate to other UI elements.

Don't do that on X, it causes all kinds of problems.

dos added a comment.May 12 2019, 6:38 PM
In D21134#464005, @dos wrote:

To anyone playing with those paddings: please note, that until T8177 is fixed, on X11 with hidpi screens you need to run Plasma with PLASMA_USE_QT_SCALING=1 env variable, as otherwise the font sizes are disproportionate to other UI elements.

Don't do that on X, it causes all kinds of problems.

I think you meant "don't forget to switch it back afterwards", as it's literally the only way to get correct paddings on X11 with hidpi.

filipf added a subscriber: filipf.Jun 1 2019, 10:10 PM
alex-l added a subscriber: alex-l.Jun 12 2019, 6:30 PM

Just a mockup but is this middle ground solution good enough?

( I also suggested to move the disappearing indicator at the bottom here )

dos added a comment.Jun 13 2019, 1:23 PM

Just a mockup but is this middle ground solution good enough?

Yeah, it looks much better than what's currently in 5.16.

GB_2 added a subscriber: GB_2.Jun 13 2019, 1:40 PM

Just a mockup but is this middle ground solution good enough?

I absolutely love it!

ngraham updated this revision to Diff 59731.Jun 13 2019, 3:08 PM

Rebase in preparation for resuming work on this

ngraham updated this revision to Diff 59741.Jun 13 2019, 6:10 PM

Address review comments

ngraham edited the test plan for this revision. (Show Details)Jun 13 2019, 6:11 PM
ngraham marked 2 inline comments as done.

All right, I *think* I've addressed the review comments and implemented something as close to @alex-l's design as I can get it.

ngraham edited the test plan for this revision. (Show Details)Jun 13 2019, 6:13 PM
ngraham updated this revision to Diff 59743.Jun 13 2019, 6:22 PM
ngraham marked an inline comment as done.

Fix RTL paddings