app-center icon indicating copy to clipboard operation
app-center copied to clipboard

feat: Add rated category snaps to the Games tab

Open ashuntu opened this issue 1 year ago โ€ข 10 comments

This PR adds an ordered list of snaps based on their rating to the Games tab. Doing this required adding support for the top charts endpoints from the ratings service to the ratings client.

I've added this as a new variation of the CategorySnapList, so hopefully if we intend to incorporate top charts into other areas of the app, we can easily reuse the RatedCategorySnapList. A spinner is displayed while the list is loading; it does take a moment since we have to fetch the snap details for each snap ID.

This is more or less what it looks like, though the actual snaps displayed will not be the same: image

UDENG-3351

ashuntu avatar Jul 12 '24 21:07 ashuntu

This is looking nice!

  • It would be helpful if you could share a screenshot of how it looks next to other sections, such as app cards, etc. Is this something you can share here?

  • The way I designed it originally was to fit 4 cols (in the wide window) to add some interesting layout to the page. So the cards are 3 columns, and this is 4. See the screenshot below (grid in pale pink)

Screenshot 2024-08-13 at 16 56 53
  • I suppose that the titles (Top rated, etc) are the same size as the ones in the Explore page, right?

  • The numbers seem too big. Can you decrease the font size?

Hope this makes sense :)

anasereijo avatar Aug 13 '24 16:08 anasereijo

I decreased the font size of the numbers from 24 to 16. Here are some additional screenshots (with the font size change): image image

And this is what it looks like fullscreen (I have a 1440p 16:10 display): image

I had some trouble fitting in 4 apps in a row without compromising the titles/descriptions, but if you'd like, I can take another look at doing that.

The titles are the same size as the other page headings, yes.

ashuntu avatar Aug 14 '24 21:08 ashuntu

Thanks @ashuntu, the smaller numbers are looking better.

If you have the capacity, I think it is worth trying to fit the charts in 4 cols on the widest screen. Looking at the screenshot seems that you have a 'card' around the app when on hover. That isn't necessary (the whole area should be clickable though) and maybe it will save up some space to fit the 4 cols? It will also decrease the padding between the number and the icon, I think.

Screenshot 2024-08-16 at 09 44 49

maybe @spydon has some ideas too.

Also, I am happy to jump in a call with you if easier to discuss this! :)

anasereijo avatar Aug 16 '24 08:08 anasereijo

How does this look? (this still includes an outline on hover)

image

ashuntu avatar Aug 16 '24 22:08 ashuntu

I think this could work, thanks for trying @ashuntu!

  • What is the padding between each column? It should match the padding used in the other sections.
  • We don't need the outline on hover. It can be just clickable but without the outline
  • Maybe is due being a screenshot, but seems that the numbers are out of the grid?: Screenshot 2024-08-20 at 10 33 38

Thank you!

anasereijo avatar Aug 20 '24 09:08 anasereijo

I removed the border on hover now.

For the columns, they should have the same padding as the other cards (they use the same grid, just have different column count and aspect ratio).

I think the numbers are slightly out of the grid in the same way that the other titles (like the F in "Featured Titles" in the screenshot you showed) are. This happens on all the pages it looks like, where the first letter in titles is slightly out of bounds. If I had to guess, that's just due to the font, but I'm not sure. Maybe someone else from the apps squad would know better than me on that.

ashuntu avatar Aug 22 '24 20:08 ashuntu

Thanks, @ashuntu! I can bring that up with the squad, as it's something not really related then to this PR.

That said, LGTM! thanks for making the changes.

anasereijo avatar Aug 27 '24 08:08 anasereijo

@d-loose I rebased and made most of the changes you pointed out.

I added the explicit mapping for chart categories, let me know if you think there is a better way than the way I chose in https://github.com/ubuntu/app-center/pull/1740/commits/73e1b4e691606a783608b3cd2c6626c2c563049c.

ashuntu avatar Oct 15 '24 21:10 ashuntu

Otherwise we'd still need to add some UI fallback state that hides the top charts in case the service doesn't support the feature.

I added fallback to the old behavior (just a regular CategorySnapList) when the ratings service for charts isn't available https://github.com/ubuntu/app-center/pull/1740/commits/23375de704a8d3d5e434c660085b8a2c09643eb2.

ashuntu avatar Oct 17 '24 15:10 ashuntu

I added a try/catch here instead for the assertions endpoint to handle the YAML errors we talked about. So for now the rated grids will just ignore any snap with invalid YAML assertions until a raw response is sorted in snapd.dart.

ashuntu avatar Oct 18 '24 14:10 ashuntu

@sminez I've pushed a gaming-top-charts branch (currently the same as main) and changed the base of this PR to target that one, so we can land additional changes before merging to main without bothering @ashuntu too much :)

d-loose avatar Dec 04 '24 11:12 d-loose

@d-loose thanks for re-pointing the PR to the new branch :ok_hand: please merge at your leisure :sunglasses:

sminez avatar Dec 04 '24 13:12 sminez