Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add rated category snaps to the Games tab #1740

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

ashuntu
Copy link
Collaborator

@ashuntu ashuntu commented Jul 12, 2024

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 ashuntu marked this pull request as ready for review July 12, 2024 22:07
@anasereijo
Copy link
Collaborator

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 :)

@ashuntu
Copy link
Collaborator Author

ashuntu commented Aug 14, 2024

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.

@anasereijo
Copy link
Collaborator

anasereijo commented Aug 16, 2024

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! :)

@ashuntu
Copy link
Collaborator Author

ashuntu commented Aug 16, 2024

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

image

@anasereijo
Copy link
Collaborator

anasereijo commented Aug 20, 2024

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!

@ashuntu
Copy link
Collaborator Author

ashuntu commented Aug 22, 2024

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.

@anasereijo
Copy link
Collaborator

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.

Comment on lines 116 to 119
style: const TextStyle(
fontSize: 16.0,
fontWeight: FontWeight.w500,
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any text style in the theme that fits here?

],
final appContent = [
AppIcon(iconUrl: iconUrl),
const SizedBox(width: 16, height: 16),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kCardSpacing maybe?

(entry) => AppCard.fromRatedSnap(
snap: entry.value,
onTap: () => onTap(entry.value),
rating: entry.key + 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the number is the key, and the snap is the value? 🤔
That seems backwards?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key in this is just from the .asMap call, so I can get the index and give each snap a number next to it based on the index (its rank). Is there a different way you would do it?

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rebase the PR? Let me know if you need any help!

packages/app_center/lib/ratings/rated_category_model.dart Outdated Show resolved Hide resolved
packages/app_center/lib/snapd/snapd_service.dart Outdated Show resolved Hide resolved
packages/app_center/lib/widgets/app_card.dart Outdated Show resolved Hide resolved
packages/app_center/lib/widgets/app_card.dart Outdated Show resolved Hide resolved
Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some testing with the staging environment @matthew-hagemann managed to deploy now :)
There's a small bug due to mismatching category definitions, see comments for details

packages/app_center/lib/ratings/ratings_service.dart Outdated Show resolved Hide resolved
@ashuntu
Copy link
Collaborator Author

ashuntu commented Oct 15, 2024

@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 73e1b4e.

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, those are great improvements! I have two more tiny nitpicks and would be happy to merge this soon :)

@sminez, @matthew-hagemann should we wait until the ratings service with support for charts is deployed before merging this? 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.

packages/app_center/lib/snapd/snapd_service.dart Outdated Show resolved Hide resolved
@ashuntu
Copy link
Collaborator Author

ashuntu commented Oct 17, 2024

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 23375de.

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! I'd still wait until tomorrow morning (European time) to merge this, if that's alright :)

@ashuntu
Copy link
Collaborator Author

ashuntu commented Oct 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants