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: improve 'Claimed' column UX in datatable #2986

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wescopeland
Copy link
Member

@wescopeland wescopeland commented Dec 26, 2024

This PR makes an improvement to the "Claimed" column and is a soft preparation for migrating the various claims table pages to React, namely with the new component shown in the screenshots below.

Before
Screenshot 2024-12-26 at 4 39 49 PM

After
Screenshot 2024-12-26 at 4 40 03 PM

The game list query now returns the active claimants for the game. In the column itself, a new component called UserAvatarStack is used.

As the name implies, this component stacks user avatars:
Screenshot 2024-12-26 at 4 42 09 PM

Screenshot 2024-12-26 at 4 42 31 PM

The column's sort order logic is unchanged: it's a simple yes or no. Mobile is unaffected by these changes.

@wescopeland wescopeland requested a review from a team December 26, 2024 21:43
@@ -34,7 +34,16 @@ private function buildBaseQuery(
?User $user = null,
?int $targetId = null,
): Builder {
$query = Game::with(['system'])
$query = Game::query()
Copy link
Member Author

Choose a reason for hiding this comment

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

query() is just here to move with() to the next line so line indentation makes sense.

{hasActiveOrInReviewClaims && row.original.game.claimants ? (
<div className="flex items-center gap-1.5">
<LuCheck className="size-4" />
<span className="sr-only">{strings.t_yes}</span>
Copy link
Member

Choose a reason for hiding this comment

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

I find the X misleading, especially right next to the add-to-list column
image

Similarly, the check mark is redundant. Either there will be one or more avatars, or there will be zero avatars. Do we really need the X/Check too?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can get rid of the check and X. Though, I do like that all of our table cells have an empty state of some kind. To maintain consistency, I've reverted the empty state for this one back to what it was previously:

Screenshot 2024-12-31 at 8 46 18 PM

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.

2 participants