Skip to content
This repository was archived by the owner on Sep 6, 2024. It is now read-only.

feat: Portal 1139/Table Component #108

Merged
ian-sawyer merged 14 commits into
mainfrom
isawyer/Portal-1139
Nov 30, 2023
Merged

feat: Portal 1139/Table Component #108
ian-sawyer merged 14 commits into
mainfrom
isawyer/Portal-1139

Conversation

@ian-sawyer

@ian-sawyer ian-sawyer commented Nov 28, 2023

Copy link
Copy Markdown
Contributor

@ian-sawyer
ian-sawyer force-pushed the isawyer/Portal-1139 branch 2 times, most recently from b1e3624 to c3c7906 Compare November 28, 2023 17:37
@ian-sawyer
ian-sawyer marked this pull request as ready for review November 28, 2023 17:38
<Table>
<TableHead>
<TableRow>
<TableHeader className="table-header-checkbox">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Having class names that are aware of the content within seems weird to me. I think it would be better of the content itself worried about it's style.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated

<Table>
<TableHead>
<TableRow>
<TableHeader className="width-05 flex-justify-center">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What of these classes can we use as defaults for the table? I would like it if, by default, the user wouldn't have to include these classes, especially the width and padding ones.

export interface TableHeaderProps {
children?: React.ReactNode | React.ReactNode[];
className?: string;
size?: "sm" | "md" | "lg";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we are using the same strings here for size in this component and the TableDataCell component, let's extract it out to a type and reuse it.

<TableBody>
{data.map((info, index) => (
<TableRow key={`table-row-${index}`}>
<TableDataCell size="sm" className="flex-justify-center">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the size prop specified here needs to match the one set in the corresponding TableHeader. Is it possible to have the data cell follow the column cell width?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants