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

Implement sticky columns in GenericTable #1023

Merged
merged 9 commits into from
Jan 16, 2025

Conversation

martha
Copy link
Contributor

@martha martha commented Jan 13, 2025

Description

GH issue: https://github.com/open-path/Green-River/issues/6761

This PR is a follow-up from this comment thread in Slack. Summary of changes:

  • Add ability in GenericTable to define columns as "sticky" on the left or right
  • Implement "sticky" columns on all the in-scope tables for 6761
  • Update to TableRowActions so that on mobile (smallest screen size), the primary action button appears as an icon. (It sounds like this may become true across the board, even on wider screens pending design decision)

I should note that, while I've once-overed every individual table that's using this new pattern, I haven't tested this PR extremely thoroughly, since the ticket is over estimate. For efficiency, I'm proposing to do more thorough QA in the QA environment once this is merged to the deploy branch.

Type of change

  • Bug fix
  • New feature (adds functionality)
  • Code clean-up / housekeeping
  • Dependency update

Checklist before requesting review

  • I have performed a self-review of my code
  • I have run the code that is being changed under ideal conditions, and it doesn't fail
  • My code includes comments and/or descriptive variable names to help other engineers understand the intent (or not applicable)
  • My code follows the style guidelines of this project (eslint)
  • I have updated the documentation (or not applicable)
  • If it's not obvious how to test this change, I have provided testing instructions in this PR or the related issue

src/components/elements/CommonMenuButton.tsx Show resolved Hide resolved
@@ -52,7 +57,6 @@ export const CASE_NOTE_COLUMNS = {
NoteContentPreview: {
key: 'content-preview',
header: 'Note Content Preview',
maxWidth: '450px',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed since this wasn't respected (maxWidth is not a valid property on ColumnDef and GenericTable doesn't do anything with it). There is a maxWidth applied to the Box below.

@@ -74,7 +74,7 @@ const BulkServicesTable: React.FC<Props> = ({
</NotCollectedText>
);
return [
CLIENT_COLUMNS.name,
{ ...CLIENT_COLUMNS.name, sticky: 'left' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Known issue: Checkbox is not sticky. I think my current approach would need a bit more work to accomplish more than 1 sticky column per side. It's not ideal, but I am proposing to descope this and open another ticket, unless you see an easy way around it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok with me @martha. Not sure how much it would simplify things, but one option is to make the checkbox always be sticky. That seems reasonable to me.

Copy link
Contributor Author

@martha martha Jan 15, 2025

Choose a reason for hiding this comment

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

Thanks, Gig, good point! With this suggestion it wasn't so bad, see 273b059

@martha martha marked this pull request as ready for review January 14, 2025 20:21
@martha martha requested a review from gigxz January 14, 2025 20:21
<Button onClick={primaryActionConfig.onClick} {...buttonProps}>
{primaryActionConfig.title}
</Button>
))}
{primaryAction}
{!!secondaryActionConfigs && secondaryActionConfigs.length > 0 && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Noting that I think there was agreement around including the primary action config inside the secondary menu, so I'd say we can go ahead with that. Maybe it would be as simple as passing items={[primaryActionConfig , ...secondaryActionConfigs]} below.

Copy link
Collaborator

@gigxz gigxz left a comment

Choose a reason for hiding this comment

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

LGTM after suggested Typescript fixes, to ensure continued type safety.

I also think we should include the row-hover-bg-color once we deploy this as well (for review) but I'm not seeing that change on the deploy branch- will that be a separate PR?

@martha
Copy link
Contributor Author

martha commented Jan 15, 2025

I also think we should include the row-hover-bg-color once we deploy this as well (for review) but I'm not seeing that change on the deploy branch- will that be a separate PR?

Yes, I wanted to keep that on a separate diff since we haven't reached consensus on that change yet, but I opened a draft PR here: #1025

@martha martha requested a review from gigxz January 15, 2025 20:01
Copy link
Collaborator

@gigxz gigxz left a comment

Choose a reason for hiding this comment

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

looks good!

@martha martha merged commit a3217f9 into deploy/6761-table-actions Jan 16, 2025
1 check passed
@martha martha deleted the martha/6761-table-sticky branch January 16, 2025 14:09
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