3.1 issue143 props typing#158
Conversation
| PropTypes.number | ||
| ]), | ||
| hidden: PropTypes.bool, | ||
| id: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Only the id is required
There was a problem hiding this comment.
what happens if name isn't supplied? I think we should make both id and name required to force our users to supply both and standardize on a convention
There was a problem hiding this comment.
We currently revert to 'id' when there is no name
| setProps: PropTypes.func, | ||
| start_cell: PropTypes.arrayOf(PropTypes.number), | ||
| style_as_list_view: PropTypes.bool, | ||
|
|
There was a problem hiding this comment.
dropdown_properties can not be typed better as it is a lightweight dictionary object for columns.
| value: PropTypes.oneOfType([ | ||
| PropTypes.number, | ||
| PropTypes.string | ||
| ]) |
There was a problem hiding this comment.
The only dropdown values that make sense at the moment are strings and numbers. We may expand on that later.
|
|
||
| interface IDropdownValue { | ||
| label: string; | ||
| value: string | number; |
There was a problem hiding this comment.
Matching props dropdown value typing
| active_cell: PropTypes.array, | ||
| columns: PropTypes.arrayOf(PropTypes.object), | ||
| columns: PropTypes.arrayOf(PropTypes.shape({ | ||
| clearable: PropTypes.bool, |
There was a problem hiding this comment.
could we add docstrings to all of these as well?
There was a problem hiding this comment.
Sure. I'll create another issue, mark it as P1, and link it to the example below.
| columns: PropTypes.arrayOf(PropTypes.object), | ||
| columns: PropTypes.arrayOf(PropTypes.shape({ | ||
| clearable: PropTypes.bool, | ||
| deletable: PropTypes.oneOfType([ |
There was a problem hiding this comment.
For example:
/**
* If True, the user can delete the column by clicking on a little `x` button on the
* column.
* If there are multi-header columns (via setting `name` to an array of strings)
* with merged column headers, then you can choose which column header row
* to display the "x" in by supplying a column row index. For example, `0` will
* display the "x" on the first row, `1` on the second row. If the "x" appears on
* a merged column, then deleting the column will delete all of the merged columns
* associated with it.
*/
| selector: PropTypes.string, | ||
| rule: PropTypes.string | ||
| selector: PropTypes.string.isRequired, | ||
| rule: PropTypes.string.isRequired |
There was a problem hiding this comment.
Added 'isRequired' to nested props that absolutely need to be present
valentijnnieman
left a comment
valentijnnieman
left a comment
There was a problem hiding this comment.
Looks good to me, not approving so that @chriddyp can continue discussing name prop if he wants
3 participants
For #143.
improve external facing props types
improve internal facing props types