Sorting + Skeleton loading state

[dan13ram]

Closes #503

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/metafam/the-game/6vJMvT6Neac1dLfd7czF7X1DYqaC
✅ Preview: https://the-game-git-feat-sorting-metafam.vercel.app

[alalonde]

Cool! Love the loading state.

I did some testing on https://the-game-9t0xeliie-metafam.vercel.app/players and noticed a few things:

• I don't think the XP label is very clear (e.g. XP: 675 (4392)), not sure if that was "officially" designed or not
• fearlessthompson is 516 (986) in the seasonal XP sort mode, but XP: 1024 in the normal sort mode.

[dan13ram]

@alalonde please take a look at https://the-game-git-feat-sorting-metafam.vercel.app
Not sure which commit that link you shared is from.

I don't think the XP label is very clear (e.g. XP: 675 (4392)), not sure if that was "officially" designed or not

This was something me and @davort came up with. Please suggest a better alternative if you have one in mind :)

fearlessthompson is 516 (986) in the seasonal XP sort mode, but XP: 1024 in the normal sort mode.

This seems like an issue with caching. A refresh and it should work fine. Now it shows 1024 for both cases for me.

[davort]

@alalonde please take a look at https://the-game-git-feat-sorting-metafam.vercel.app
Not sure which commit that link you shared is from.

I don't think the XP label is very clear (e.g. XP: 675 (4392)), not sure if that was "officially" designed or not

This was something me and @davort came up with. Please suggest a better alternative if you have one in mind :)

Indeed, this was a low cost fix for showing seasonal XP, while still providing info on total XP in the same view. For the sake of clarity, we would have them show separately, and with accompanying text labels: SEASONAL XP: 123 TOTAL XP: 1000

And when sorted by total XP, show only TOTAL XP: 1000

If this isn't too much work, I'd suggest going with that. It will make the cards longer, since the XP metadata will most likely take up another row, if formatted like this. We may want to compensate by shortening the "ABOUT" text to three rows, so as to not elongate the cards further, and make scanning the page easier. Let me know your thoughts on this idea please, @alalonde and @dan13ram

Finally, one thing I noticed, that happens for me at least, is that the cards in the same row aren't of the same height

@dan13ram – is it just me or do you notice the same? If it's the same for you too, could you please take a look into that?

[alalonde]

Indeed, this was a low cost fix for showing seasonal XP, while still providing info on total XP in the same view. For the sake of clarity, we would have them show separately, and with accompanying text labels: SEASONAL XP: 123 TOTAL XP: 1000

Yup, this sounds good to me. Do we display what the current season is somewhere? A name, dates, etc

[alalonde]

Noticed this too, if it's not a regression I can just create another issue

[alalonde]

Looks good overall, I have some minor concerns about some of these shared components starting to do too much. Probably not worth blocking an approval / merge.

Apologies for the delay in review.

[alalonde]

This sort of logic should probably not be in a component in the design-system package as those components are intended to be generic and reusable, no? Could you e.g. pass in the value of tagLabel instead?

[dysbulic]

If I load the test site and click on "Sorted By" → "Seasonal XP", I get an error.

2021-08-09@06.08.11.mp4

[lucidcyborg]

The error comes up for all the sorting options when it's already checked.

Also, can you make the dropdown close when selecting an option?
For me it stays open until I click somewhere else

[lucidcyborg]

I'll create a separate issue for the close on click. So this can be merged

[dan13ram]

@alalonde @dysbulic @mquellhorst Thank you for your reviews! Apologies for taking this long to fix the PR.
Please take a look now and let me know if its good to merge.

[alalonde]

Thanks! Created #786 for the issue @dysbulic mentioned