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(PHC-4380): row selection to TableModule #333

Merged
merged 2 commits into from
Mar 23, 2023
Merged

Conversation

shawnzhu
Copy link
Member

It uses the row selection API from tanstack to simplify developers' learning curve.

This is an temporary solution before fully adopting the TanStack table API via #331

@shawnzhu shawnzhu added the enhancement New feature or request label Mar 23, 2023
Copy link
Contributor

@MMFane MMFane 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! A couple small pieces of feedback, and I'll approve after the few type errors blocking merge are resolved.

src/components/TableModule/types.ts Outdated Show resolved Hide resolved
src/components/TableModule/TableModule.stories.tsx Outdated Show resolved Hide resolved
},
cell: {
content: (rowData: any) => (
<Checkbox
Copy link
Contributor

Choose a reason for hiding this comment

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

If you think it's not too much for this PR, could you add a class to the checkbox here that makes its click area larger for improved UX? Here it is for your reference:

checkboxClass: {
    padding: theme.spacing(1.5),
    margin: `calc(-1 * ${theme.spacing(1.5)})`,
  },

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to add something to the TableModule.stories.tsx but there's no theme so just need to figure out how to use this snippet

Copy link
Contributor

Choose a reason for hiding this comment

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

I see some other components that leverage a theme in /src/styles. IMO, every table should have the same spacing here as a default and maybe it should be extensible, but I'm guessing most cases will want the basic spacing (or we may want to just enforce this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a temp solution before deprecating the table in favor of one that leverages tanStack's we can go without this for now

src/components/TableModule/TableModule.tsx Outdated Show resolved Hide resolved
@@ -412,6 +427,51 @@ export const TableModule = React.memo(
}
};

const isRowSelected = (row: any, index: number): boolean => {
Copy link
Contributor

Choose a reason for hiding this comment

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

again, double checking that this any (and the following one) are definitely what we want

@shawnzhu shawnzhu merged commit 4029016 into master Mar 23, 2023
@shawnzhu shawnzhu deleted the PHC-4380-TableModule branch March 23, 2023 19:03
@github-actions
Copy link

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants