-
Notifications
You must be signed in to change notification settings - Fork 116
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
The Mockito version was updated since the last mocks were generated, these are the same but regenerated to satisfy CI.
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?
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