Differential D17355

Align Plasma QML button content in center if it has an icon
AbandonedPublic
Actions

Authored by GB_2 on Dec 4 2018, 6:47 PM.

Details

Reviewers
None
Group Reviewers
Plasma
VDG
Summary

Aligns the Plasma QML Button content in the center if it has an icon.
It makes this look better: https://phabricator.kde.org/D17323

Before:

After:

Test Plan

Run this test QML file with qmlscene:

main.qml264 BDownload

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Lint Skipped
Unit
Unit Tests Skipped
GB_2 created this revision.Dec 4 2018, 6:47 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 4 2018, 6:47 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
GB_2 requested review of this revision.Dec 4 2018, 6:47 PM
ngraham added a subscriber: ngraham.Dec 4 2018, 7:11 PM

Cool!

Does this still work properly when the text is so long that the button expands to accommodate it?

Now that I think about this a bit, it seems like maybe the problem here is that there's a minimum button width in the first place. If we changed that so that buttons could be narrower when the icon + label is small, than we probably wouldn't need to horizontally center anything.

GB_2 updated this revision to Diff 46862.EditedDec 4 2018, 7:27 PM

It works properly, no matter how big the button is or if it has an icon or not.
In my opinion we should leave it how it is now.

apol added a subscriber: apol.Dec 4 2018, 11:21 PM
apol added inline comments.
src/declarativeimports/plasmastyle/ButtonStyle.qml
41 ↗(On Diff #46862)

This looks wrong, a RowLayout inside another one?

davidedmundson added a subscriber: davidedmundson.Dec 4 2018, 11:24 PM

Code wise:

  • Any changes to PlasmaStyle need to happen in the QQC2 (PlasmaComponents3) version too before shipping
  • Please be sure to run qmlscene tests/components/button.qml
  • Why do we have nested RowLayouts?

If we changed that so that buttons could be narrower when the icon + label is small, than we probably wouldn't need to horizontally center anything.

That's do-able from application space.

Button {

implicitWidth: minimumWidth

}

If you want that, there's no need to change this

GB_2 updated this revision to Diff 46907.Dec 5 2018, 4:48 PM

Improved code.
How qmlscene tests/components/button.qml looks like:


For some reason "elide" doesn't work...

GB_2 marked an inline comment as done.Dec 5 2018, 4:49 PM
davidedmundson added a comment.Dec 5 2018, 4:51 PM

On the button with menu, the triangle has moved. That seems an unwanted change.

GB_2 updated this revision to Diff 46918.EditedDec 5 2018, 8:44 PM

I can't figure out how to make "elide" work without using Layout.fillWidth.

davidedmundson added a comment.Dec 5 2018, 8:56 PM

I can't figure out how to make "elide" work without using Layout.fillWidth.

You won't be able to. You need that.
(or setting a max width, which effectively is the same thing)

GB_2 added a comment.Dec 5 2018, 9:09 PM

Hmm... then I give up.

GB_2 updated this revision to Diff 46920.EditedDec 5 2018, 9:16 PM

Improved menu button.

davidedmundson added a comment.Dec 8 2018, 11:16 PM

Any changes to PlasmaStyle need to happen in the QQC2 (PlasmaComponents3) version too before shipping

src/declarativeimports/plasmastyle/ButtonStyle.qml
41 ↗(On Diff #46862)

why this Item wrapper

mart added a subscriber: mart.Dec 14 2018, 4:13 PM

in general, buttons with icons may be stacked vertically, which for me makes a general -1

mart added a comment.Dec 14 2018, 4:14 PM
In D17355#371722, @GB_2 wrote:

Improved code.
How qmlscene tests/components/button.qml looks like:


For some reason "elide" doesn't work...

is elide working on the latest revision?

mart added a comment.Dec 14 2018, 4:15 PM

pleaase add the changes to plasmacomponents3 as well

GB_2 planned changes to this revision.Dec 14 2018, 6:48 PM
In D17355#376973, @mart wrote:

in general, buttons with icons may be stacked vertically, which for me makes a general -1

But it's not consistent with Qt Widgets and looks kind of weird.

In D17355#376975, @mart wrote:
In D17355#371722, @GB_2 wrote:

Improved code.
How qmlscene tests/components/button.qml looks like:


For some reason "elide" doesn't work...

is elide working on the latest revision?

No, I still didn't figure that out.

In D17355#376976, @mart wrote:

pleaase add the changes to plasmacomponents3 as well

I will do that when this is done.

This needs revision, but it's not really my priority.

GB_2 abandoned this revision.Apr 3 2019, 2:38 PM

Revision Contents
Changeset List

Diff 46907

View Options

src/declarativeimports/plasmastyle/ButtonStyle.qml

Loading...