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

💄 Pin actions column on user table #393

Closed
wants to merge 1 commit into from
Closed

Conversation

JMicheli
Copy link
Collaborator

This pull request addresses an issue reported in #372. Although that issue has been closed with the addition of a scroll bar to the table, this pull request additionally pins the actions column on the user table to the right so that it will always be visible regardless of where the table has been scrolled to.

Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

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

I added basic support for pinned columns using the Table component, but it wasn't a finished implementation (see this TODO comment). The TLDR; is that there is no background on the action cell, which doesn't provide enough contrast if we were at a viewport which places the action above another cell.

See example
Screen.Recording.2024-08-12.at.7.12.48.AM.mov

I think this can be merged as-is, but wanted to put that context up there in case you were interested in working on the Table component further

@JMicheli
Copy link
Collaborator Author

I'm interested in making sure this is done right - I saw the TODO but I didn't really understand the steps it was suggesting needed to be taken.

Could you describe, at a high level, what you're thinking needs to be done to Table here?

@aaronleopold
Copy link
Collaborator

Could you describe, at a high level, what you're thinking needs to be done to Table here?

I think for the short term, a few smaller changes in a few places of the table component could work without rewriting the whole thing to be modular (which that comment alludes to):

  • I think there is currently padding on both the table and the cell:
    • Remove padding on table, this will cause issues where elements are visible behind the cell
    • Place padding on cells (including header cells) instead: first:pl-3 if not pinned left and last:pr-3 if not pinned right
  • The background needs to be set for pinned columns, but the table is currently unaware of what that should be. For example, the background would be bg-background but in the cards it is bg-background-200. So it needs a way to pass the class. I think the short term could just be adding a tableCellClassName prop and manually passing last:bg-background-200 in this instance

Offhand that's all I can think of

@aaronleopold
Copy link
Collaborator

I added changes relating to this here: cc2b67e. I'm going to close this out in favor of the linked PR

@aaronleopold aaronleopold deleted the pin-user-actions branch August 18, 2024 20:47
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