D21134: [Notifications] Tweak paddings

2019-06-13 Thread Nathaniel Graham 
ngraham updated this revision to Diff 59743.
ngraham marked an inline comment as done.
ngraham added a comment.


  Fix RTL paddings

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21134?vs=59741&id=59743

BRANCH
  arcpatch-D21134

REVISION DETAIL
  https://phabricator.kde.org/D21134

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationPopup.qml

To: ngraham, #vdg, broulik
Cc: GB_2, alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, 
LeGast00n, ericadams, jraleigh, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-13 Thread Nathaniel Graham 
ngraham marked 2 inline comments as done.
ngraham added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: GB_2, alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, 
LeGast00n, ericadams, jraleigh, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-13 Thread Nathaniel Graham 
ngraham updated this revision to Diff 59741.
ngraham added a comment.


  Address review comments

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21134?vs=59731&id=59741

BRANCH
  arcpatch-D21134

REVISION DETAIL
  https://phabricator.kde.org/D21134

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationPopup.qml

To: ngraham, #vdg, broulik
Cc: GB_2, alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, 
LeGast00n, ericadams, jraleigh, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-13 Thread Nathaniel Graham 
ngraham updated this revision to Diff 59731.
ngraham added a comment.


  Rebase in preparation for resuming work on this

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21134?vs=57885&id=59731

BRANCH
  arcpatch-D21134

REVISION DETAIL
  https://phabricator.kde.org/D21134

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationPopup.qml

To: ngraham, #vdg, broulik
Cc: GB_2, alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, 
LeGast00n, ericadams, jraleigh, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-13 Thread Björn Feber 
GB_2 added a comment.


  In D21134#478796 , @alex-l wrote:
  
  > Just a mockup but is this middle ground solution good enough?
  
  
  I absolutely love it!

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: GB_2, alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, 
LeGast00n, ericadams, jraleigh, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-13 Thread Sebastian Krzyszkowiak 
dos added a comment.


  In D21134#478796 , @alex-l wrote:
  
  > Just a mockup but is this middle ground solution good enough?
  
  
  Yeah, it looks much better than what's currently in 5.16.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, LeGast00n, 
ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-06-12 Thread Alessandro Longo 
alex-l added a comment.


  Just a mockup but is this middle ground solution good enough?
  
  F6885918: immagine.png 
  
  ( I also suggested to move the disappearing indicator at the bottom here 
 )

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: alex-l, filipf, prettyvanilla, cfeck, dos, apol, plasma-devel, LeGast00n, 
ericadams, jraleigh, GB_2, ragreen, Pitel, ZrenBot, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-12 Thread Sebastian Krzyszkowiak 
dos added a comment.


  In D21134#464175 , @broulik wrote:
  
  > 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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: cfeck, dos, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-12 Thread Kai Uwe Broulik 
broulik added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: cfeck, dos, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-12 Thread Christoph Feck 
cfeck added inline comments.

INLINE COMMENTS

> NotificationItem.qml:74
>  
> -property int headingLeftPadding: 0
> +property int headingLeftPadding: units.smallSpacing
>  property int headingRightPadding: 0

Does this take into account layouts for RTL languages?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: cfeck, dos, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-12 Thread Sebastian Krzyszkowiak 
dos added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: dos, apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-11 Thread Kai Uwe Broulik 
broulik added a comment.


  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.
  F6818773: Screenshot_20190511_094009.png 

  The padding of the screenshot doesn't match the rest of the notification now
  F6818770: Screenshot_20190511_094220.png 

  The right padding doesn't match anymore either
  F6818775: Screenshot_20190511_094424.png 

  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
  F6818778: Screenshot_20190511_094550.png 

  
  Code is fine I guess.

INLINE COMMENTS

> NotificationItem.qml:263
>  Layout.fillWidth: true
> +Layout.leftMargin: units.smallSpacing
> +Layout.rightMargin: units.smallSpacing

Why is this not `bodyLeftPadding`?

> NotificationItem.qml:265
> +Layout.rightMargin: units.smallSpacing
> +Layout.bottomMargin: units.smallSpacing
>  active: notificationItem.notificationType === 
> NotificationManager.Notifications.JobType

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

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-10 Thread Aleix Pol Gonzalez 
apol added a comment.


  +1 I too am missing some spacing.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D21134

To: ngraham, #vdg, broulik
Cc: apol, plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, mart

D21134: [Notifications] Tweak paddings

2019-05-10 Thread Nathaniel Graham 
ngraham created this revision.
ngraham added reviewers: VDG, broulik.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
ngraham requested review of this revision.

REVISION 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
  [screenshots go here]

REPOSITORY
  R120 Plasma Workspace

BRANCH
  tweak-notification-paddings (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D21134

AFFECTED FILES
  applets/notifications/package/contents/ui/NotificationItem.qml
  applets/notifications/package/contents/ui/NotificationPopup.qml

To: ngraham, #vdg, broulik
Cc: plasma-devel, jraleigh, GB_2, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart