D9542: Improve display of technical app info
This revision was automatically updated to reflect the committed changes. Closed by commit R134:bc2e71d3a877: Improve display of technical app info (authored by ngraham). REPOSITORY R134 Discover Software Store CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9542?vs=24470=24489 REVISION DETAIL https://phabricator.kde.org/D9542 AFFECTED FILES discover/qml/ApplicationPage.qml To: ngraham, apol, #vdg, #discover_software_store Cc: colomar, januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham added a comment. We already had everything in the main content area before we had a real toolbar on top, at which point the icon and app name were moved there. I'll land this, and then we can continue iterating as necessary. REPOSITORY R134 Discover Software Store BRANCH master REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: colomar, januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
apol accepted this revision. This revision is now accepted and ready to land. REPOSITORY R134 Discover Software Store BRANCH master REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: colomar, januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
colomar added a comment. This layout makes more sense to me as well overall. The only thing that I find a bit strange is that the summary now looks a bit like a headline, especially given that the application name is in the toolbar instead of the content area. I'm wondering if we should move the name and icon into the content area. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: colomar, januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
alexeymin added a comment. I don't spot any obvious problems in QML any more. And I like newer version more than previous... REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham edited the test plan for this revision. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham updated this revision to Diff 24470. ngraham marked 2 inline comments as done. ngraham added a comment. - Use a larger font instead of bold for the caption - Put the homepage on top, too REPOSITORY R134 Discover Software Store CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9542?vs=24456=24470 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9542 AFFECTED FILES discover/qml/ApplicationPage.qml To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham added a comment. I could do a bigger font instead of bold, sure. As for going around in circles: sorry, I wasn't around a few releases ago, so I didn't know what it looked like back then. But folks do seem to think this is nicer than the status quo... REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
apol added a comment. We used to have exactly this same layout for that section few releases ago, let's not keep doing the same changes over and over. Also I'm not a big fan of the bold. Maybe it would make sense to make the font slightly bigger? REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham added a comment. Good idea, I'll do that too. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
januz added a comment. +1 Definitely an improvement. What do you think about grouping the homepage with the rest of the info at the top? REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: januz, alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham marked 2 inline comments as done.
ngraham added inline comments.
INLINE COMMENTS
> alexeymin wrote in ApplicationPage.qml:153
> Why do you put a `Text` inside a label? Why not use `font.bold` property of
> Label itself?
This was actually the only way I could get it to work. Doing this:
QQC2.Label {
Layout.fillWidth: true
text: appInfo.application.comment
font.bold: true
wrapMode: Text.WordWrap
elide: Text.ElideRight
maximumLineCount: 1
bottomPadding: 20
}
...had no effect, and did not make the text bold.
REPOSITORY
R134 Discover Software Store
REVISION DETAIL
https://phabricator.kde.org/D9542
To: ngraham, apol, #vdg, #discover_software_store
Cc: alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham updated this revision to Diff 24456. ngraham added a comment. - Merge branch 'master' of git.kde.org:discover - Address review comments REPOSITORY R134 Discover Software Store CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9542?vs=2=24456 BRANCH master REVISION DETAIL https://phabricator.kde.org/D9542 AFFECTED FILES discover/qml/ApplicationPage.qml To: ngraham, apol, #vdg, #discover_software_store Cc: alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
alexeymin added a comment.
"After" definitely looks better :)
INLINE COMMENTS
> ApplicationPage.qml:153
> Layout.fillWidth: true
> -text: appInfo.application.comment
> +Text {
> +text: appInfo.application.comment
Why do you put a `Text` inside a label? Why not use `font.bold` property of
Label itself?
> ApplicationPage.qml:166
> +columns: 2
> Layout.fillWidth: true
> +
Does `Layout.fillWidth` have an effect here? Shouldn't it be inside of
QQC2.Label block?
> ApplicationPage.qml:189
> +elide: Text.ElideRight
> +text: version ? i18n("%1", version) : ""
> +}
Translation is probably not needed here now
REPOSITORY
R134 Discover Software Store
REVISION DETAIL
https://phabricator.kde.org/D9542
To: ngraham, apol, #vdg, #discover_software_store
Cc: alexeymin, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham edited the summary of this revision. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham edited the test plan for this revision. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
D9542: Improve display of technical app info
ngraham edited the test plan for this revision. REPOSITORY R134 Discover Software Store REVISION DETAIL https://phabricator.kde.org/D9542 To: ngraham, apol, #vdg, #discover_software_store Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart
