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

Table rfc #458

Closed
wants to merge 9 commits into from
Closed

Table rfc #458

wants to merge 9 commits into from

Conversation

dswho
Copy link
Contributor

@dswho dswho commented Nov 6, 2018

This is a technical proposal for baseUI regarding the API design of a basic table, which handles the canonical table format of one header row and many rows of data. Any other table formats (e.g. having rows or columns that may span multiple rows or columns can be handled using a renderProp)

Also, there will be support for sorting.

I had a question about what was the best way to determine how many rows are displayed initially in the table. Let's say the user has 5000 rows to load into a table. I was thinking about arbitrarily setting a pageSize of 10, such that 10 rows are displayed initially, and then on user scroll or click of a pagination element, more elements are displayed. This lends the question: should a table come with a scrollbar and/or pagination element out of the box? I currently don't have this documented in the proposal but needed further discussion.

@CLAassistant
Copy link

CLAassistant commented Nov 6, 2018

CLA assistant check
All committers have signed the CLA.

rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated
## Exports

* `Table`
* `StatefulTable`
Copy link
Contributor

Choose a reason for hiding this comment

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

What state does the StatefulTable handles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally included StatefulTable, but since I wasn't able to find a need for it yet, I'll remove it until there is a need.

rfcs/table.md Outdated
}
render = () => {
return (
<Table
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there should be some loading state in the table with remote data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I'll add an isLoading prop to the table.

rfcs/table.md Outdated

## `Column` API
One of the Table columns prop for describing the table's columns, Column has the same API.
* `title: string` - Required. Title of the column to display in the header.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about React.Node instead of a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good suggestion, in someone wants to display custom html in the header.

rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated
* `dataIndex: string` - Optional. Attribute at which to index the row data.
* `render: <T>(<T>, index, Array<T>) => $ReactNode` - Optional. Renderer of the table cell. The return value should be a ReactNode.
* `sorter:Function`- Optional Sort function for [local sort](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort)
* `defaultSortOrder:'ascend'|'descend'`: Only to be declared on one of the columns, this will be define the default sort on the initial set of data.
Copy link

Choose a reason for hiding this comment

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

Why not a prop of the Table component? To avoid the ambiguity of defaultSortOrder defined on multiple columns.

rfcs/table.md Outdated Show resolved Hide resolved
rfcs/table.md Outdated
## Accessibility

How can this component be used via keyboard controls?
No keyboard shortcuts will be enabled.
Copy link

Choose a reason for hiding this comment

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

Might be nice to enable highlighting via keyboard? e.g. click in a cell, table gains keyboard focus, move the highlight with arrow keys. I'm ambivalent...also, this would add local state, which we might want to avoid if possible.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I'll add it to the RFC

rfcs/table.md Outdated
will be rendered.

The [W3 guideline for table states](https://www.w3.org/WAI/tutorials/tables/):
> Header cells must be marked up with `<th>`, and data cells with `<td>` to make tables accessible. For more complex tables, explicit associations may be needed using `scope`, `id`, and `headers` attributes.
Copy link

Choose a reason for hiding this comment

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

Ahhh...this is certainly a benefit of sticking to the HTML table spec...hm. I do see ARIA roles on react-virtualized, at least: bvaughn/react-virtualized#606

Choose a reason for hiding this comment

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

react-virtualized does add aria-* roles & sets tabIndex to assist with keyboard nav.
It also provides some additional components ie: ArrowKeyStepper to help with navigating virtualized lists

rfcs/table.md Outdated
}
};

class TableWithPagination extends React.Component {

Choose a reason for hiding this comment

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

As per our convo aswell yesterday might be good to include what pagination looks like with infinite scrolling. IE async requests on scrolling table.

rfcs/table.md Outdated Show resolved Hide resolved
@chasestarr
Copy link
Collaborator

closing this to continue conversation in #632

@chasestarr chasestarr closed this Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants