Table

[Apatil15]

I have created first rfc using template for table component. Please review it and let me know your feedback/suggestions

https://github.com/wwnorton/design-system-rfcs/blob/table/accepted/0001-component-table.md

[sh0ji]

This design isn't 100% clear to me, but I think that could be improved quite a bit with examples that tell more of a story. For instance, how would a developer create a sortable table that's sorted by last name on first load? How would they do the same thing with JSON data?

I'll also update the template because I think you highlighted some areas where it could be more clear.

[sh0ji]

This shouldn't be about alternatives on the market, it should be about alternative designs.

You could put an alternative here like, "Extend the Ant Design table" where the design is essentially an NDS <Table> element that wraps the Ant Design one and then I would expect an explanation for why you chose not to do that here. What benefits does your API have over extending an existing one? (things like, "we would be dependent on the development of Ant in order to implement new features" would be a good argument)

[sh0ji]

And to be even more clear, I'd expect other APIs that you may have considered like using render functions for a rows prop on the <Table>, along with an explanation of why you chose not to go that route.

[sh0ji]

Let's just remove this section.

[sh0ji]

This should be drawbacks to the design, not the feature set.

For instance, if the design doesn't allow for paging to be implemented later on or makes it difficult to do in user-space (at the application level), you should explain why that is here. The fact that it doesn't currently support paging isn't really a drawback unless something about the design makes paging impossible or difficult to implement.

[Apatil15]

@sh0ji I have made the changes as per the given suggestion please review it, also added my comments on suggestions.

[wguevara-norton]

I think it would be better if the sorting is a string or a string array containing the desired values for this function, such as: ascending, descending, or unsorted.

[sh0ji]

I actually think we need two props to sort a two dimensional array (a table):

1. A prop to indicate direction ("ascending"/"descending")
2. A prop to indicate the column that's being sorted (the name of the column?)

Internally, sort needs to go on the <th> element (https://www.w3.org/TR/wai-aria-practices-1.2/#gridAndTableProperties_sort), so an alternative would be to put sort be a prop that goes on a <TableCell> when it's in the context of a <TableHeader>. For example:

<Table>
  <TableHeader>
    <TableCell>First name</TableCell>
    <TableCell sort="ascending">Last name</TableCell>
  </TableHeader>
  <TableRow>
    <TableCell>Marissa</TableCell>
    <TableCell>Keep</TableCell>
  </TableRow>
  <TableRow>
    <TableCell>Andrew</TableCell>
    <TableCell>Arnold</TableCell>
  </TableRow>
</Table>

At the top level, you could create a sort context and then set it inside the table row or cell to accomplish this with React.

[sh0ji]

Where'd you land on this, @Apatil15? I see that the type is now "variant", but that type isn't defined anywhere.

My opinion is that if we have sort on the <Table>, it should be of type "ascending" | "descending" (not the abbreviated "asc"/"desc") and it should be required that a sortBy prop also be included for it to have any effect.

Name	Type	Description	Required	Default
sort	"ascending" | "descending"	Indicates the direction of sort. Must be used in conjunction with sortBy to sort the table.	false	undefined
sortBy	string	The column key that the table should be sorted by. Must be used in conjunction with sort to sort the table.	false	undefined

[wguevara-norton]

<Table>
  <TableHeader>
      <Column>First Name</Column>
      <Column>Last Name</Column>
  </TableHeader>
  <Row>
    <Column>Marissa</Column>
    <Column>Keep</Column>
  </Row>
  <Row>
    <Column>Andrew</Column>
    <Column>Arnold</Column>
  </Row>
</Table>

[wguevara-norton]

tableCell => column

[sh0ji]

This design still doesn't seem complete to me and most of my comments haven't been addressed. My expectation is that you respond to them and we have a discussion, eventually resolving the conversation. If you make a change that directly addresses a suggestion I made, you can click the "resolve conversation" button yourself.

But I also have two broad concerns that I think are more important than the code-level comments I made.

1. The composition and data APIs need to be talked about separately.
2. Data types need to be defined.

Composition and Data APIs

This is my biggest concern because there seem to be two APIs here: a data-driven API (dataSource, dataColumn), and a compositional API. My recommendation would be to separate the two designs conceptually and illustrate how they work together.

For instance, here's a quick example of what I'm imagining, somewhat based on your design and someone deviating from it. You can use some of it, but I'm absolutely not saying it has to be this design—I just want to get us on the right track.

// composition API
<Table>
	<TableHeader>
		<TableCell>First name</TableCell>
		<TableCell key="last_name" sort="ascending">Last name</TableCell>
	</TableHeader>
	<TableRow>
		<TableCell>Marissa</TableCell>
		<TableCell>Keep</TableCell>
	</TableRow>
	<TableRow>
		<TableCell>Andrew</TableCell>
		<TableCell>Arnold 🎩</TableCell>
	</TableRow>
</Table>

// data-driven API
<DataTable
	header={[
		/**
		 * Simple form: a string for the name. Internally, it's transformed
		 * into the { name, key, sort, etc. } form with all defaults and a
		 * key that's just the string in camelCase form.
		 */
		'First name',
		/**
		 * Complete form: a header cell can be a full object to customize
		 * things such as sorting or to use a custom key.
		 */
		{
			name: 'Last name',
			key: 'last_name',
			sort: 'ascending',
		},
	]}
	rows={[
		/**
		 * Simplest form: a tuple that maps to the order of the header cells.
		 */
		['Marissa', 'Keep'],
		/**
		 * Object form (simple): an unordered set of key: value pairs that
		 * correspond to the keys given in the header.
		 */
		{ firstName: 'Andrew', last_name: 'Arnold' },
		/**
		 * Complete form: a collection that allows you to customize some part
		 * of individual cells.
		 */
		[
			{ key: 'firstName', value: 'Andrew' },
			{
				// key: 'lastName', // if not specified, use the position in the array?
				value: 'Arnold',
				// give Mr. Arnold that hat with a render function!
				render: (cell: React.ReactNode) => (
					<>
						{ cell }
						{ ' ' }
						🎩
					</>
				),
			},
		],
	]}
/>

And then the data-driven API would simply be concerned with validating the data, normalizing it, and then rendering it with the composition API:

const DataTable = ({ header, rows, ...options }) => (
	<Table {...options}>
		<TableHeader>
			{ header.map(headerMappingFunction) }
		</TableHeader>
		{ rows.map(rowMappingFunction) }
	</Table>
);

Data types

While applications may be retrieving data in JSON, I assume that they're parsing it so it's in an object form of some kind when it's consumed by the design system component. We'll have to define the types of those objects and it's best to do it at this stage.

Here's an example of what that would look like for my above example using header and rows as data-driven props:

// using generics to allow key validation and type checking
interface DataTableRowCell<K extends string = string> {
	name: string;
	key?: K;
	render?: (node: React.ReactNode) => React.ReactNode;
}

// this assumes that a header cell's signature is the same as a rows but with additional props
interface DataTableHeaderCell<K extends string = string> extends DataTableRowCell<K> {
	sort?: 'ascending' | 'descending';
}

// an intermediary interface that joins the various ways to set a row
type DataTableRow<K extends string = string> = (DataTableRowCell<K> | string)[] | Record<K, string>;

// the main actual type for the <DataTable> example from above
export interface DataTableProps {
	header: (DataTableHeaderCell | string)[];
	rows: DataTableRow[];
}

[sh0ji]

This should either be sticky or it should be a prop on <Table> called stickyHeader.

<Table stickyHeader />

// or:
<Table>
	<TableHeader sticky />
</Table>

And would there be any reason to support both? For instance, if you're using <Table> as a data table rather than a compositional table?

[Apatil15]

StickyHeader props support both table and TableHeader component, Table props carry forward to TableHeader internally.

[sh0ji]

Gotcha. Which one "wins" if I do this?

<Table stickyHeader>
	<TableHeader stickyHeader={false} />
</Table>

And let's name the prop on <TableHeader> just sticky even if it's inherited. I don't love the redundancy of <TableHeader stickyHeader />.

[Apatil15]

@sh0ji I have addressed all the suggestion also added my comments for the same, please review it.

[sh0ji]

Thanks for your patience with all the feedback on this, @Apatil15. I think this is basically ready to implement, but I have one request: let's expose two top-level table components.

• <Table> - the interface for composition. In other words, you would use this component to construct a structured table, similar to HTML. This component shouldn't have any props for setting content in its interface* or have any built-in way to use a data-driven approach. <TableHeader>, <TableRow>, and <TableCell> should be part of this API.
• <DataTable> - the interface for the data props. It should wrap <Table> and handle anything related to data normalization or validation. Only <DataTable> should be part of this API. In other words, it shouldn't have a children prop.

This will allow us to keep data-related functionality separate from composition functionality and give users two options for setting tables, depending on how they're getting the data for the table.

* Technically, <TableCell> should have a children prop, which should be the only way to set table content with the composition API.

[Apatil15]

@sh0ji Done documentation as per the suggestion and started implementation for table component.

[sh0ji]

Thanks, @Apatil15! Merging our first RFC. 🎉 🚢