Differential D23984 Diff 66244 sidebar/package/contents/ui/introPage.qml

Changeset View

Standalone View

View Options

sidebar/package/contents/ui/introPage.qml

Show All 19 Lines
20import QtQuick.Controls 2.5 as QQC2 20​import QtQuick.Controls 2.5 as QQC2
21import QtQuick.Layouts 1.1 21​import QtQuick.Layouts 1.1
22 22​
23import org.kde.kirigami 2.5 as Kirigami 23​import org.kde.kirigami 2.5 as Kirigami
24 24​
25Rectangle { 25​Rectangle {
26 id: root 26​ id: root
27 color: Kirigami.Theme.backgroundColor 27​ color: Kirigami.Theme.backgroundColor
28​ property int verticalMargin: 50
ngrahamUnsubmitted
Not Done

How about Kirigami.Units.gridUnit * 3 instead?

ngraham: How about `Kirigami.Units.gridUnit * 3` instead?
28 29​
ngrahamUnsubmitted
Not Done

Don't hardcode values here; instead use multiples of Kirigami.Units.gridUnit (or some other defined unit)

ngraham: Don't hardcode values here; instead use multiples of Kirigami.Units.gridUnit (or some other…
29 signal focusNextRequest() 30​ signal focusNextRequest()
30 signal focusPreviousRequest() 31​ signal focusPreviousRequest()
31 32​
32 function focusFirstChild() { 33​ function focusFirstChild() {
33 iconsRow.children[0].focus = true; 34​ iconsRow.children[0].focus = true;
34 } 35​ }
35 36​
36 function focusLastChild() { 37​ function focusLastChild() {
37 iconsRow.children[iconsRow.children.length-1].focus = true; 38​ iconsRow.children[iconsRow.children.length-1].focus = true;
38 } 39​ }
39 40​
40 ColumnLayout { 41​ RowLayout {
41 anchors { 42​ anchors {
42 bottom: separator.top 43​ bottom: parent.verticalCenter
43 bottomMargin: Kirigami.Units.largeSpacing 44​ bottomMargin: verticalMargin
44 horizontalCenter: parent.horizontalCenter 45​ horizontalCenter: parent.horizontalCenter
45 } 46​ }
47​ Layout.alignment: Qt.AlignHCenter
46 Kirigami.Icon { 48​ Kirigami.Icon {
47 Layout.alignment: Qt.AlignHCenter 49​ Layout.alignment: Qt.AlignHCenter
48 source: "preferences-system" 50​ source: "preferences-system"
49 width: Kirigami.Units.iconSizes.enormous 51​ width: Kirigami.Units.iconSizes.enormous
50 height: width 52​ height: width
51 opacity: 0.3 ​
52 } 53​ }
53 QQC2.Label { 54​ Item {
55​ width: Kirigami.Units.largeSpacing
ngrahamUnsubmitted
Not Done

Comment not really necessary; it's obvious that this is a spacer

ngraham: Comment not really necessary; it's obvious that this is a spacer
56​ }
57​ ColumnLayout {
58​ RowLayout {
59​ Kirigami.Icon {
54 Layout.alignment: Qt.AlignHCenter 60​ Layout.alignment: Qt.AlignHCenter
anthonyfieroniUnsubmitted
Done

Use i18n also only Plasma about me.

anthonyfieroni: Use i18n also only Plasma about me.
61​ source: "start-here-kde"
anthonyfieroniUnsubmitted
Not Done

Don't hardcode font sizes use level instead
https://api.kde.org/frameworks-api/frameworks-apidocs/frameworks/kirigami/html/classorg_1_1kde_1_1kirigami_1_1Heading.html#ad694a087926d5fd83edc183b526db4a4

anthonyfieroni: Don't hardcode font sizes use level instead https://api.kde.org/frameworks-api/frameworks…
ngrahamUnsubmitted
Not Done

Just use level 1, it's big enough IMO (or make the icon next to the text smaller to compensate for the smaller text size)

ngraham: Just use level 1, it's big enough IMO (or make the icon next to the text smaller to compensate…
62​ width: Kirigami.Units.iconSizes.medium
63​ height: width
64​ }
65​ Kirigami.Heading {
davidedmundsonUnsubmitted
Done

Hardcoding sizes is generally frowned upon.

Partly because things are user-configurable, partly because if every dev hardcodes their own sizes we'll have an inconsistent mess everywhere.

This patch has quite a few. Try and see what you can get rid of

davidedmundson: Hardcoding sizes is generally frowned upon. Partly because things are user-configurable…
martUnsubmitted
Done

if it's a kirigami.heading, it should use the automatic sizes from there.
All headings having the same size across everywhere is much more important than a perfect pixel alignment on a single place
(which won't really work anyways, because we are dealing with fonts and pojn ts there)

mart: if it's a kirigami.heading, it should use the automatic sizes from there. All headings having…
66​ text: i18n("Plasma")
67​ level: 1
68​ font.weight: Font.Bold
69​ }
70​ }
71​
72​ Kirigami.Heading {
55 text: i18n("System Settings") 73​ text: i18n("System Settings")
74​ level: 1
56 } 75​ }
57 } 76​ }
58 Kirigami.Separator { ​
59 id: separator ​
60 anchors.centerIn: parent ​
61 width: Math.round(parent.width * 0.8) ​
62 } 77​ }
63 ColumnLayout { 78​ Kirigami.Heading {
ngrahamUnsubmitted
Not Done

Why move this heading out of the ColumnLayout?

ngraham: Why move this heading out of the ColumnLayout?
79​ id: frequentlyUsed
64 anchors { 80​ anchors {
65 top: separator.bottom 81​ top: parent.verticalCenter
66 topMargin: Kirigami.Units.largeSpacing 82​ topMargin: verticalMargin
67 horizontalCenter: parent.horizontalCenter 83​ horizontalCenter: parent.horizontalCenter
68 } 84​ }
69 width: Math.round(parent.width * 0.8) ​
70 Kirigami.Heading { ​
71 Layout.alignment: Qt.AlignHCenter 85​ Layout.alignment: Qt.AlignHCenter
72 level: 3 86​ level: 3
73 wrapMode: Text.NoWrap 87​ wrapMode: Text.NoWrap
74 text: i18n("Frequently used:") 88​ text: i18n("Frequently used:")
75 } 89​ }
90​
91​ ColumnLayout {
92​ anchors {
93​ top: frequentlyUsed.bottom
94​ topMargin: Kirigami.Units.largeSpacing
95​ horizontalCenter: parent.horizontalCenter
96​ }
97​ width: parent.width * 0.8
ngrahamUnsubmitted
Not Done

No need to add an id since nothing references it

ngraham: No need to add an id since nothing references it
ngrahamUnsubmitted
Not Done

Why change this? Rounding generally fixes the problem of blurriness with fractional scaling due to non-integer pixels.

ngraham: Why change this? Rounding generally fixes the problem of blurriness with fractional scaling due…
GB_2Unsubmitted
Not Done

Maybe change to "Frequently Used", so it's a header?

GB_2: Maybe change to "Frequently Used", so it's a header?
76 RowLayout { 98​ RowLayout {
77 id: iconsRow 99​ id: iconsRow
78 Layout.fillWidth: true 100​ Layout.fillWidth: true
79 Layout.alignment: Qt.AlignHCenter 101​ Layout.alignment: Qt.AlignHCenter
80 spacing: Kirigami.Units.largeSpacing 102​ spacing: Kirigami.Units.largeSpacing
martUnsubmitted
Done

add in a comment a link to https://en.wikipedia.org/wiki/Relative_luminance to explain the numbers

mart: add in a comment a link to https://en.wikipedia.org/wiki/Relative_luminance to explain the…
davidedmundsonUnsubmitted
Done

See https://doc.qt.io/qt-5/qml-qtqml-qt.html#darker-method

davidedmundson: See https://doc.qt.io/qt-5/qml-qtqml-qt.html#darker-method
martUnsubmitted
Done

right, since you are using brightness only to have a darker color, you should use Qt.darker()

mart: right, since you are using brightness only to have a darker color, you should use Qt.darker()
81 property int spaceForIcon: Math.max((iconsRow.parent.width - iconsRow.spacing * 4) / 5, Kirigami.Units.iconSizes.medium) 103​ property int spaceForIcon: Math.max((iconsRow.parent.width - iconsRow.spacing * 4) / 5, Kirigami.Units.iconSizes.medium)
82 property int iconSize: iconsRow.spaceForIcon >= Kirigami.Units.iconSizes.huge 104​ property int iconSize: iconsRow.spaceForIcon >= Kirigami.Units.iconSizes.huge
83 ? Kirigami.Units.iconSizes.huge 105​ ? Kirigami.Units.iconSizes.huge
84 : (iconsRow.spaceForIcon >= Kirigami.Units.iconSizes.large ? Kirigami.Units.iconSizes.large : Kirigami.Units.iconSizes.medium) 106​ : (iconsRow.spaceForIcon >= Kirigami.Units.iconSizes.large ? Kirigami.Units.iconSizes.large : Kirigami.Units.iconSizes.medium)
85 107​
davidedmundsonUnsubmitted
Done

It's best to infer this from the child.

davidedmundson: It's best to infer this from the child.
86 Repeater { 108​ Repeater {
87 id: mostUsedRepeater 109​ id: mostUsedRepeater
88 model: systemsettings.mostUsedModel 110​ model: systemsettings.mostUsedModel
89 delegate: IntroIcon { 111​ delegate: IntroIcon {
90 icon: model.decoration 112​ icon: model.decoration
91 text: model.display 113​ text: model.display
92 iconSize: iconsRow.iconSize 114​ iconSize: iconsRow.iconSize
93 Layout.minimumWidth: iconsRow.spaceForIcon 115​ Layout.minimumWidth: iconsRow.spaceForIcon
94 Layout.maximumWidth: Layout.minimumWidth 116​ Layout.maximumWidth: Layout.minimumWidth
95 visible: (index + 1) * iconSize + index * iconsRow.spacing < iconsRow.parent.width 117​ visible: (index + 1) * iconSize + index * iconsRow.spacing < iconsRow.parent.width
96 } 118​ }
97 } 119​ }
98 } 120​ }
99 } 121​ }
122​
100} 123​}