Differential D13237

Show action.main more prominently on the ToolBarApplicationHeader
ClosedPublic
Actions

Authored by apol on May 31 2018, 1:56 PM.

Details

Reviewers
mart
ngraham
Group Reviewers
Kirigami
Commits
R169:cf62d95943e8: Show action.main more prominently on the ToolBarApplicationHeader
Summary

Show it as a full button instead of a tool button.
Use the icon color as the button color.

Diff Detail

Repository
R169 Kirigami
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
apol created this revision.May 31 2018, 1:56 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptMay 31 2018, 1:56 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.May 31 2018, 1:56 PM
ngraham requested changes to this revision.May 31 2018, 2:03 PM
ngraham added a subscriber: ngraham.

Love it! Two issues that I can see:

  • In this example, the text really needs to be white; it doesn't show up well against the dark green background. If the color is programmatically determined, we may need some fancy logic to choose the button's text color to ensure that it's visible enough. For that matter, the icon doesn't show up very well either...
  • The button has no right padding, and touches the window edge ( A Discover issue?)
This revision now requires changes to proceed.May 31 2018, 2:03 PM
apol added a comment.May 31 2018, 2:07 PM
In D13237#271434, @ngraham wrote:

Love it! Two issues that I can see:

  • In this example, the text really needs to be white; it doesn't show up well against the dark green background. If the color is programmatically determined, we may need some fancy logic to choose the button's text color to ensure that it's visible enough. For that matter, the icon doesn't show up very well either...

Yeah... thought so too. If people are not against the idea it's something to spend time on. I'm afraid it could look weird though...

  • The button has no right padding, and touches the window edge ( A Discover issue?)

No, still in kirigami, it's just that with a non-flat button we don't take into account the padding within the button as a margin. We can add a margin.

apol updated this revision to Diff 35255.May 31 2018, 2:12 PM

Address some typos and added nate's margin

mart added inline comments.May 31 2018, 2:27 PM
src/controls/ToolBarApplicationHeader.qml
105

can just assign flat: false instead of adding a new property in actiontoolbutton?

mart requested changes to this revision.May 31 2018, 4:33 PM
mart added inline comments.
src/controls/private/PrivateActionToolButton.qml
62–67

note that this can't be used directly, as it depends from Qt 5.10
while if the button is colored by the method in D13232 this shouldn't happen
so this shouldn't be pushed until can be done without breaking deps

This revision now requires changes to proceed.May 31 2018, 4:33 PM
apol updated this revision to Diff 35296.Jun 1 2018, 12:00 AM

Remove property, only assign the palette if there's a palette property

mart accepted this revision.Jun 1 2018, 11:39 AM
davidedmundson added a subscriber: davidedmundson.Jun 1 2018, 11:48 AM

Please remove console.log()

This revision was not accepted when it landed; it landed in state Needs Review.Jun 1 2018, 3:05 PM
Closed by commit R169:cf62d95943e8: Show action.main more prominently on the ToolBarApplicationHeader (authored by apol). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 35296

View Options

src/controls/ToolBarApplicationHeader.qml

Loading...
View Options

src/controls/private/PrivateActionToolButton.qml

Loading...