[Table] Add Regions/IRegion docs #1666
Conversation
cmslewis
requested review from
llorca,
themadcreator,
gscshoyru and
tgreenwatts
Add Region docsPreview: documentation | table |
Delete unnecessary IRegion description linePreview: documentation | table |
|
@llorca it repros on blueprintjs.com, so this PR isn't the source. But file an issue if you want to track. 👍 |
| @@ -89,12 +89,21 @@ export type ICellInterval = [number, number]; | |||
| export type ICellCoordinate = [number, number]; | |||
|
|
|||
| /** | |||
| * A ZERO-indexed region of cells. | |||
There was a problem hiding this comment.
Moved this knowledge into the props descriptions.
packages/table/src/docs.md
Outdated
| [`Table.scrollToRegion`](#table-js.instance-methods) instance method. | ||
|
|
||
| There are four different types of regions, each represented by a different | ||
| `RegionCardinality`: |
There was a problem hiding this comment.
how is this object used in code? you mention it here but never show examples that use it.
There was a problem hiding this comment.
maybe it would make more sense to introduce this enum after explaining IRegion? be like, "ok now that you know all that, here's the shorthand we developed for it."
packages/table/src/docs.md
Outdated
| const multiColumnRegion = Regions.column(0, 2); // { rows: null, cols: [0, 2] } | ||
| const multiRowRegion = Regions.row(0, 2); // { rows: [0, 2], cols: null } | ||
|
|
||
| const tableRegion = Regions.table(); // { rows: null, cols: null } |
There was a problem hiding this comment.
how do these functions relate to RegionCardinality?
packages/table/src/regions.ts
Outdated
| * The first and last column indices in the region, inclusive and | ||
| * zero-indexed. If `cols` is `null`, then all columns are understood to be | ||
| * included in the region. | ||
| */ | ||
| cols?: ICellInterval; |
There was a problem hiding this comment.
perhaps add | null to these types to match docs? or does it support undefined (cuz of optional prop)?
I know we don't use sNC in blueprint but these annotations are useful in the outside world.
There was a problem hiding this comment.
maybe the type should be cols: ICellInterval | null? not optional, just nullable?
There was a problem hiding this comment.
Totally agree, but I'm hesitant to make that change in an otherwise docs-only PR. Might break people who are manually constructing their regions if they happened to pass undefined.
There was a problem hiding this comment.
Added | null but left the ? for now to keep allowing undefined. The existing code and comments are actually out-of-sync on this point: code uses undefined but comments mention null. Can fix later.
Respond to CR feedbackPreview: documentation | table |
Fixes #902
Checklist
Changes proposed in this pull request:
Tabledocumentation now includes a section about Regions and theIRegioninterface.Reviewers should focus on: