[OverlaySheet] Add bottom margin when sheet height is less than 3/4 of screen
Needs ReviewPublic

Authored by cblack on Apr 7 2020, 6:02 PM.

Details

Reviewers
None
Group Reviewers
Kirigami
Summary

A 2gu margin is added if the sheet's content height is less than 3/4 of the screen

BUG: 419804
FIXED-IN: 5.70

Diff Detail

Repository
R169 Kirigami
Branch
cblack/sheet-scroll-fix (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25086
Build 25104: arc lint + arc unit
cblack created this revision.Apr 7 2020, 6:02 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptApr 7 2020, 6:02 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
cblack requested review of this revision.Apr 7 2020, 6:02 PM
cblack added a subscriber: ngraham.

Thanks for the patch, but why would you need the bottom margin to be customizable? Shouldn't the default be to simply center the sheet vertically when it's not as tall as the window, as (IIRC) it was before?

src/controls/templates/OverlaySheet.qml
108

missing whitespace around the times sign

284

missing whitespace around the plus sign

304

ditto

mart added a subscriber: mart.Apr 9 2020, 7:53 AM

Thanks for the patch, but why would you need the bottom margin to be customizable? Shouldn't the default be to simply center the sheet vertically when it's not as tall as the window, as (IIRC) it was before?

it was around the lower third.. now is attached to the bottom edge of the screen, which some user feedback seems to prefer it.

maaaybe, what's actually needed is a verticalAlign property?

I think having the overlaysheet glued to the bottom is fine for a very tall sheet that's scrollable or almost scrollable. But when it's not very tall, having it vertically centered makes more sense IMO. A short sheet glued to the bottom looks very weird to me. Rather than exposing yet another setting here, I think automatically choosing an alignment based on height would make more sense.

cblack updated this revision to Diff 79890.Apr 11 2020, 10:05 PM

Use heuristics instead of exposing new API

cblack updated this revision to Diff 79891.Apr 11 2020, 10:06 PM

Add whitespace

cblack retitled this revision from [OverlaySheet] add bottomMargin property to [OverlaySheet] Add bottom margin when sheet height is less than 3/4 of screen.Apr 11 2020, 10:08 PM
cblack edited the summary of this revision. (Show Details)

I think it would be better to vertically center in this case instead of adding a fixed bottom margin. The result with the patch in its current state is better, but IMO could still be improved:

Don't you think that might look better vertically centered?

I think it would be better to vertically center in this case instead of adding a fixed bottom margin. The result with the patch in its current state is better, but IMO could still be improved:

Don't you think that might look better vertically centered?

I mean, if you want that, that's when you would use a dialog instead of a sheet.

Well, one of the ideas of Kirigami is to not spawn new windows when not needed. Maybe the basic QQC2 popup (which does open centered) needs to gain the same visual style as an OverlaySheet. Or maybe OverlaySheet should be able to be used as a centered pop-up thingy.

@mart, your thoughts?

Well, one of the ideas of Kirigami is to not spawn new windows when not needed.

The Dialog in QQC2 (not to be confused with the dialog from QtQuick Dialogs) isn't a separate window. It's a modal overlay on top of the current window, like any of the other Popup family members.

Okay. But then the difference between a sheet and a dialog is inherently nebulous: a short sheet is like a dialog; a tall dialog is like a sheet. In my mind it makes sense to use one thing for both and tweak the presentation accordingly. Clearly lots of people are using OverlaySheet as a dialog.

mart added a comment.EditedApr 22 2020, 7:16 AM

so, let's try to vertically center when the size is less than the parent

mart added a comment.Apr 23 2020, 12:58 PM

in the end, just centered vertically the sheet in 812238afdcb7