Display star ratings
ClosedPublic

Authored by ngraham on Apr 1 2018, 11:30 PM.

Details

Summary

For quite some time, users and reviewers have been asking for Discover to display star ratings. This is really useful information, and becomes especially important now that we have multiple sort modes. People will think, "this list may be sorted by rating, but what ARE the ratings?"

This patch adds star ratings to the browse and search lists, and the detailed view pages.

It also adjusts the look of the delegates to better accommodate stars without becoming cluttered.

FEATURE: 389601
FIXED-IN: 5.13.0

Depends on D11842, because we need the extra space gained by removing the less-useful categories.

Test Plan

Before:

After, Applications page:

After, Installed page:

After, Applications Page in compact or mobile view:

Diff Detail

Branch
show-star-ratings (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham requested review of this revision.Apr 1 2018, 11:30 PM
ngraham created this revision.
ngraham edited the summary of this revision. (Show Details)Apr 1 2018, 11:32 PM
ngraham edited the test plan for this revision. (Show Details)
ngraham set the repository for this revision to R134 Discover Software Store.
Restricted Application added a project: Plasma. · View Herald TranscriptApr 1 2018, 11:32 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

Nice placement too. I really, really like how this looks.

ngraham edited the summary of this revision. (Show Details)Apr 3 2018, 4:14 PM
ngraham added a reviewer: VDG.Apr 4 2018, 1:04 PM

VDG input requested. Do folks think we should also drop the blue line? Here's how the lists would look:

Or perhaps gray instead of blue? I think that conforms to our HIG better, which says that color should be used for action.

ragreen added a subscriber: ragreen.Apr 4 2018, 1:19 PM

VDG input requested. Do folks think we should also drop the blue line?
Or perhaps gray instead of blue?

No line at all +1

Yeah I like it without since they are already on separate cards.

fabianr added a subscriber: fabianr.Apr 4 2018, 1:23 PM

+1 for no line

And if there must be a line, please choose a different color. Plasma blues is supposed to be used to indicate possible interaction .

How about this, folks?

ngraham updated this revision to Diff 31295.Apr 4 2018, 1:43 PM

Update design

ngraham edited the summary of this revision. (Show Details)Apr 4 2018, 1:45 PM
ngraham edited the test plan for this revision. (Show Details)

I like the bigger stars, but then again I seem to increase font size and whatnot for a lot of things.

Yea/nay on this design? Should we tweak it a bit more?

rkflx added a subscriber: rkflx.Apr 7 2018, 11:40 AM

+1 for the concept and a version without the line and the bigger stars everywhere:

More comments:

  • Having the stars roughly the same height as the text next to it (like in the screenshot above, from an earlier iteration) works good enough for me. You might want to check how this looks with the default fonts size, though, because I suspect you bumped yours up a bit. Or could both heights be linked together?
  • As for the horizontal line, I don't think it is necessary, because the stars and the grey rating text already provide some kind of separation between title and the text below.
  • I don't like how you made the titles bold in the latest version, I think that's too much. Either bold titles and a regular font size, or no bold titles and the larger font size, but not both.
  • Looking at random screenshots of the app store on macOS and GNOME's software center, both don't have such large titles like Discover has. Might be worth for a separate patch to play around with the size a bit, to make the list less screamy and less mobiley.
ngraham updated this revision to Diff 31629.Apr 7 2018, 11:53 PM

Restore original design, but without the blue line

ngraham edited the test plan for this revision. (Show Details)Apr 7 2018, 11:58 PM
  • Having the stars roughly the same height as the text next to it (like in the screenshot above, from an earlier iteration) works good enough for me. You might want to check how this looks with the default fonts size, though, because I suspect you bumped yours up a bit. Or could both heights be linked together?

The star height uses the point size of the header, currently. It only looks like it's linked to the label size. If we want to link it to the label size, there would need to be some more work done (in another patch, obviously) regarding how the star size is calculated.

ngraham updated this revision to Diff 31646.Apr 8 2018, 4:02 AM

Revert accidental whitespace change

rkflx added a comment.Apr 8 2018, 6:42 AM

Should we tweak it a bit more?

One more thing to look into as a follow-up (besides the font size of the title) could be the vertical alignment of the text, i.e. how the first and last line relate to the icon, the button and the surrounding box. Currently there is neither centering nor aligning of any sorts:

Should we tweak it a bit more?

One more thing to look into as a follow-up (besides the font size of the title) could be the vertical alignment of the text, i.e. how the first and last line relate to the icon, the button and the surrounding box. Currently there is neither centering nor aligning of any sorts:

Yep, it's on my to-do list!

rkflx added a comment.Apr 8 2018, 4:35 PM

@ngraham Thanks for the version bump. KF 5.45 is not even released yet, but after compiling Kirigami master I was able to run Discover with your patch.

FYI, after clicking on Installed there is now some vertical overflowing going on.

apol accepted this revision.Apr 8 2018, 5:20 PM
This revision is now accepted and ready to land.Apr 8 2018, 5:20 PM
ngraham updated this revision to Diff 31694.Apr 8 2018, 8:07 PM

Fix the overflow on the Installed page by not showing the stars there (it's not relevant on that page)

ngraham updated this revision to Diff 31697.Apr 8 2018, 8:38 PM

Also fix the general case for Compact view, which also has the side effect of fixing the alignment and centering, yay!

ngraham edited the test plan for this revision. (Show Details)Apr 8 2018, 8:40 PM

Thanks for your review, @rkflx. I've fixed the overflow issue on Installed page, and in the Compact view more generally (Installed uses the Compact view). A by-product of these fixes is that the alignment in general is now fixed as well!

ngraham edited the test plan for this revision. (Show Details)Apr 8 2018, 8:47 PM
rkflx added a comment.Apr 8 2018, 10:51 PM

Thanks for your review, @rkflx. I've fixed the overflow issue on Installed page, and in the Compact view more generally (Installed uses the Compact view). A by-product of these fixes is that the alignment in general is now fixed as well!

Umm, this was more of a drive-by comment, did not test properly. Anyway, since @apol seems happy, the patch can probably land.

apol accepted this revision.Apr 8 2018, 10:57 PM
This revision was automatically updated to reflect the committed changes.