D11197: Allow OverlaySheet clients to omit the built-in close button
ngraham closed this revision. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11197 To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
ngraham added a comment. Ping REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11197 To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
ngraham updated this revision to Diff 29359. ngraham added a comment. Make the property a simple alias REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11197?vs=29130&id=29359 BRANCH overlay-sheet-without-close-button (branched from master) REVISION DETAIL https://phabricator.kde.org/D11197 AFFECTED FILES src/controls/templates/OverlaySheet.qml To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
mart requested changes to this revision. mart added a comment. This revision now requires changes to proceed. I'm ok with it, tough i would change a bit the logic, as described INLINE COMMENTS > OverlaySheet.qml:117 > + */ > +property bool showCloseButton: true > + property alias showCloseButton: closeIcon.visible > OverlaySheet.qml:304 > z: 3 > -visible: !Settings.isMobile > +visible: !Settings.isMobile && showCloseButton > width: Units.iconSizes.smallMedium with the alias then we again have only: visible: !Settings.isMobile in this case, the default behavior of showCloseButton will be !Settings.isMobile, and then the developer could always override the behavior, breaking the binding to fore it always true or always false (so the property will work even on mobile) REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11197 To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
ngraham added a comment. "Close button + cancel" is inherently redundant; a historical relic of the fact that dialog boxes have traditionally been separate windows, and windows have titlebars with fixed button arrangements (that said, in macOS, dialog boxes now have no window decorations). Having two visible methods of doing the exact same thing is pointless and confusing, and IMHO there's no reason to go out of our way to replicate this error now that we're using a convergent UI toolkit that has inline sheets. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11197 To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
apol added a comment. I'd say it's okay to have the close button and a cancel, every dialog does. REPOSITORY R169 Kirigami REVISION DETAIL https://phabricator.kde.org/D11197 To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
D11197: Allow OverlaySheet clients to omit the built-in close button
ngraham updated this revision to Diff 29130. ngraham added a comment. Fix whitespace REPOSITORY R169 Kirigami CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D11197?vs=29125&id=29130 BRANCH overlay-sheet-without-close-button (branched from master) REVISION DETAIL https://phabricator.kde.org/D11197 AFFECTED FILES src/controls/templates/OverlaySheet.qml To: ngraham, mart, #kirigami Cc: plasma-devel, apol, davidedmundson, mart, hein
