D22564: [RFC] [SwipeListItem] Take into account scrollbar visibility and form factor

[Nathaniel Graham]

ngraham updated this revision to Diff 62313.
ngraham added a comment.


  Make the scrollbar test work (thanks @mart)

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22564?vs=62116=62313

BRANCH
  respect-scrollbar-visibility-for-swipelistitem (branched from master)

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

AFFECTED FILES
  src/controls/templates/SwipeListItem.qml

To: ngraham, mart, #kirigami
Cc: apol, plasma-devel, fbampaloukas, domson, dkardarakos, davidedmundson, 
mart, hein

D22564: [RFC] [SwipeListItem] Take into account scrollbar visibility and form factor

[Aleix Pol Gonzalez]

apol added a comment.


  +1

REPOSITORY
  R169 Kirigami

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

To: ngraham, mart, #kirigami
Cc: apol, plasma-devel, fbampaloukas, domson, dkardarakos, davidedmundson, 
mart, hein

D22564: [RFC] [SwipeListItem] Take into account scrollbar visibility and form factor

[Nathaniel Graham]

ngraham updated this revision to Diff 62116.
ngraham marked an inline comment as done.
ngraham added a comment.


  Remove unnecessary `!settings.isMobile` checks

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22564?vs=62068=62116

BRANCH
  respect-scrollbar-visibility-for-swipelistitem (branched from master)

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

AFFECTED FILES
  src/controls/templates/SwipeListItem.qml

To: ngraham, mart, #kirigami
Cc: apol, plasma-devel, fbampaloukas, domson, dkardarakos, davidedmundson, 
mart, hein

D22564: [RFC] [SwipeListItem] Take into account scrollbar visibility and form factor

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> SwipeListItem.qml:241
>  verticalCenter: parent.verticalCenter
> -rightMargin: Units.gridUnit
> +rightMargin: !Settings.isMobile && behindItem.view && 
> behindItem.view.T2.ScrollBar && behindItem.view.T2.ScrollBar.vertical && 
> behindItem.view.T2.ScrollBar.vertical.visible ? 
> behindItem.view.T2.ScrollBar.vertical.width : Units.smallSpacing
>  }

Wouldn't checking for scrollbar visibility be enough? if there's a scrollbar 
but isMobile we still want the different spacing.

REPOSITORY
  R169 Kirigami

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

To: ngraham, mart, #kirigami
Cc: apol, plasma-devel, fbampaloukas, domson, dkardarakos, davidedmundson, 
mart, hein

D22564: [RFC] [SwipeListItem] Take into account scrollbar visibility and form factor

[Nathaniel Graham]

ngraham created this revision.
ngraham added reviewers: mart, Kirigami.
Herald added a project: Kirigami.
Herald added a subscriber: plasma-devel.
ngraham requested review of this revision.

REVISION SUMMARY
  Right now, `SwipeListItem` isn't as convergent as it could be. It always uses 
a very
  large margin between actions, and its right margin doesn't take into account 
the
  visibility of the scrollbar, so it's always too much.
  
  This patch makes the spacing conditional on form factor, and takes into 
account the
  scrollbar.
  
  Note: the scrollbar part of the patch doesn't actually work and always goes 
down the
  codepath for not having a scrollbar visible. It's copied from implementation 
of the
  drag handle, where it also doesn't work.I couldn't figure out how to fix it. 
Help
  would be appreciated, or else we could land this and then fix them both in a
  subsequent patch.

TEST PLAN
  [no scrollbar visible]
  Before: F7052481: Before.png 
  
  After: F7052482: After.png 

REPOSITORY
  R169 Kirigami

BRANCH
  respect-scrollbar-visibility-for-swipelistitem (branched from master)

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

AFFECTED FILES
  src/controls/templates/SwipeListItem.qml

To: ngraham, mart, #kirigami
Cc: plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, 
mart, hein