-
Notifications
You must be signed in to change notification settings - Fork 115
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
base: main
Are you sure you want to change the base?
Conversation
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. maybe @spydon has some ideas too. Also, I am happy to jump in a call with you if easier to discuss this! :) |
I think this could work, thanks for trying @ashuntu!
Thank you! |
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. |
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. |
style: const TextStyle( | ||
fontSize: 16.0, | ||
fontWeight: FontWeight.w500, | ||
), |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this 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!
There was a problem hiding this 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
74d9f8d
to
955fcc3
Compare
Enum categories need to be explicitly mapped to chart categories since the enums are not identical.
There was a problem hiding this 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.
I added fallback to the old behavior (just a regular |
There was a problem hiding this 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 :)
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. |
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 theRatedCategorySnapList
. 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:
UDENG-3351