[Table] Add menu to change cell content in Table playground #1229

cmslewis · 2017-06-13T19:44:32Z

Changes proposed in this pull request:

Add <select> menu to change cell content in the entire table in one fell swoop.
- Empty
- Cell names (A1, J17, etc.)
- Long text (random alphanumeric strings between 5 and 40 chars long)
Delete the old "Fill with random text" menu items in column headers.

Reviewers should focus on:

This only affects the table preview page.
We really need some way to force update all cells in the Table, without requiring scrolling (this goes for any Table, not just our preview). This constraint is why you have to scroll before seeing the cell content change.

Screenshot

blueprint-bot · 2017-06-13T19:51:25Z

Delete 'Fill with random text' option

Preview: documentation | table
Coverage: core | datetime

giladgray · 2017-06-13T21:37:08Z

packages/table/preview/index.tsx

+const ALPHANUMERIC_CHARS = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
+
+interface ICellContentGeneratorDictionary {
+    [key: string]: (rowIndex?: number, columnIndex?: number) => string;


key: CellContent?

Seems to require [key: number | string] :/

how about Record<CellContent, (signature) => string>?

Record is TS's shorthand for this, a la Pick or Partial.

Doesn't work:

[ts] Type 'CellContent' does not satisfy the constraint 'string'. enum CellContent

Using enums as keys is a huge discussion: microsoft/TypeScript#2491. I'll just get rid of the type safety; it's only an example page.

giladgray · 2017-06-13T21:39:23Z

packages/table/preview/index.tsx

+    private toCellContentLabel(cellContent: CellContent) {
+        if (cellContent === "CellNames") { return "Cell names"; }
+        if (cellContent === "Empty") { return "Empty"; }
+        if (cellContent === "LongText") { return "Long text"; }


why not just use the english string as the enum constant?

Given that I need to make way for an upcoming usage that will map TruncatedPopoverMode values to string labels, maybe I'll just change CellContent to a number-based enum too.

giladgray · 2017-06-13T21:40:57Z

packages/table/preview/index.tsx

+        if (isNextStateDefined &&
+            nextState.cellContent === this.state.cellContent
+            && nextState.numRows === this.state.numRows
+            && nextState.numCols === this.state.numCols


inconsistent && alignment here. put all at the beginning of lines.

giladgray · 2017-06-13T21:41:51Z

packages/table/preview/index.tsx

+
+        const cellContent = isNextStateDefined ? nextState.cellContent : this.state.cellContent;
+        const numRows = isNextStateDefined ? nextState.numRows : this.state.numRows;
+        const numCols = isNextStateDefined ? nextState.numCols : this.state.numCols;


👎 either use this.state as default value for argument, or deconstruct all at once like so:

const { ... } = isNextStateDefined ? nextState : this.state;

giladgray · 2017-06-13T21:42:43Z

packages/table/preview/index.tsx

@@ -468,6 +528,31 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
    // State updates
    // =============

+    // designed to be called from componentWillMount and componentWillUpdate, hence it expects nextProps
+    private syncCellContent = (nextState?: IMutableTableState) => {


(nextState = this.state) => { ?

That would cause the equality checks below to return on componentWillMount (because the default value would cause isNextStateDefined to never be false); I do want to go through all the content generation at that time, so I'll presumably need to know if the real nextState is defined or not.

you could pull that isNextStateDefined check up a level to componentWillUpdate itself, cuz it only applies to that one usage of this function.

blueprint-bot · 2017-06-13T22:59:50Z

CR feedback

Preview: documentation | table
Coverage: core | datetime

tgreenwatts · 2017-06-13T23:02:33Z

This has the same rendering issue, where you change the selection but have to scroll before the data shows up?
Next up - truncated text, wrapped text & json wrapped text??

Very exciting!

giladgray · 2017-06-13T23:22:34Z

packages/table/preview/index.tsx

+    EMPTY,
+    CELL_NAMES,
+    LONG_TEXT,
+};


logically, i'd define this immediately before CELL_CONTENTS instead of inserting this MutableTableState in the way.

giladgray · 2017-06-13T23:22:46Z

packages/table/preview/index.tsx

+    CellContent.EMPTY,
+    CellContent.CELL_NAMES,
+    CellContent.LONG_TEXT,
+] as CellContent[];


is this cast necessary now that it's an enum? if so, def preferable as type declaration instead of cast.

const CELL_CONTENTS: CellContent[] = ...

giladgray · 2017-06-13T23:24:06Z

packages/table/preview/index.tsx

+const ALPHANUMERIC_CHARS = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
+
+interface ICellContentGeneratorDictionary {
+    [key: string]: (rowIndex?: number, columnIndex?: number) => string;


how about Record<CellContent, (signature) => string>?

Record is TS's shorthand for this, a la Pick or Partial.

giladgray · 2017-06-13T23:26:20Z

packages/table/preview/index.tsx

+        if (cellContent === CellContent.CELL_NAMES) { return "Cell names"; }
+        if (cellContent === CellContent.EMPTY) { return "Empty"; }
+        if (cellContent === CellContent.LONG_TEXT) { return "Long text"; }
+        return "";


switch? 🤷‍♂️

giladgray · 2017-06-13T23:27:53Z

packages/table/preview/index.tsx

@@ -468,6 +528,31 @@ class MutableTable extends React.Component<{}, IMutableTableState> {
    // State updates
    // =============

+    // designed to be called from componentWillMount and componentWillUpdate, hence it expects nextProps
+    private syncCellContent = (nextState?: IMutableTableState) => {


you could pull that isNextStateDefined check up a level to componentWillUpdate itself, cuz it only applies to that one usage of this function.

giladgray · 2017-06-13T23:28:29Z

packages/table/preview/index.tsx

        return handleNumberChange((value: number) => {
            this.setState({ [stateKey]: value });
        });
    }

+    private handleStringStateChange = (stateKey: keyof IMutableTableState) => {
+        return handleStringChange((value: string) => {


def don't need the typing on value param in all three cases here, handle*Change does that for you.

then you could make these bodies into one-liners:

private handleNumberStateChange = (stateKey: keyof IMutableTableState) => { return handleNumberChange((value) => this.setState({ [stateKey]: value })); }

giladgray · 2017-06-13T23:30:30Z

packages/table/preview/index.tsx

+    private generateRandomAlphanumericString(minLength = 5, maxLength = 40) {
+        const randomLength = Math.floor(minLength + (Math.random() * (maxLength - minLength)));
+        return Utils.times(randomLength, () => {
+            const randomIndex = Math.floor(Math.random() * maxLength);


this doesn't seem safe... did you mean ALPHANUMERIC_CHARS.length?

blueprint-bot · 2017-06-14T00:17:27Z

Respond to CR feedback

Preview: documentation | table
Coverage: core | datetime

giladgray · 2017-06-14T00:20:04Z

packages/table/preview/index.tsx

+        [CellContent.CELL_NAMES]: (row: number, col: number) => Utils.toBase26Alpha(col) + (row + 1),
+        [CellContent.EMPTY]: () => "",
+        [CellContent.LONG_TEXT]: () => this.generateRandomAlphanumericString(),
+    };


seems like a good candidate for const above, no need for private member it seems. could inline generateRandomAlphanumericString for the third one.

blueprint-bot · 2017-06-14T00:38:56Z

More CR feedback

Preview: documentation | table
Coverage: core | datetime

cmslewis added 4 commits June 13, 2017 11:23

Add cell-content menu, refactor

dd28d6b

Change cell content based on selection

02301fc

Add option for long text

797f5bf

Delete 'Fill with random text' option

fe29c10

cmslewis added Domain: testing Package: table labels Jun 13, 2017

cmslewis requested review from giladgray, themadcreator and tgreenwatts June 13, 2017 19:44

Use Utils.times instead of for loop

ea990c5

giladgray suggested changes Jun 13, 2017

View reviewed changes

CR feedback

c82b6b7

giladgray reviewed Jun 13, 2017

View reviewed changes

Respond to CR feedback

a99d1b0

giladgray approved these changes Jun 14, 2017

View reviewed changes

More CR feedback

1dcf7ea

cmslewis merged commit 171da7f into master Jun 14, 2017

cmslewis deleted the cl/table-example-generate-content branch June 14, 2017 01:28

cmslewis mentioned this pull request Jun 22, 2017

Prepare Release 1.21.0 #1270

Merged

This was referenced Sep 16, 2021

[Snyk] Security upgrade webpack-cli from 3.3.12 to 4.0.0 majacQ/blueprint#54

Open

[Snyk] Fix for 1 vulnerabilities majacQ/blueprint#55

Open

snyk-bot mentioned this pull request Dec 25, 2022

[Snyk] Security upgrade webpack-cli from 3.3.12 to 4.0.0 majacQ/blueprint#192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table] Add menu to change cell content in Table playground #1229

[Table] Add menu to change cell content in Table playground #1229

cmslewis commented Jun 13, 2017 •

edited

Loading

blueprint-bot commented Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

blueprint-bot commented Jun 13, 2017

tgreenwatts commented Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

giladgray Jun 13, 2017

cmslewis Jun 13, 2017

blueprint-bot commented Jun 14, 2017

giladgray Jun 14, 2017

blueprint-bot commented Jun 14, 2017

[Table] Add menu to change cell content in Table playground #1229

[Table] Add menu to change cell content in Table playground #1229

Conversation

cmslewis commented Jun 13, 2017 • edited Loading

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

blueprint-bot commented Jun 13, 2017

Delete 'Fill with random text' option

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jun 13, 2017

CR feedback

tgreenwatts commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jun 14, 2017

Respond to CR feedback

Choose a reason for hiding this comment

blueprint-bot commented Jun 14, 2017

More CR feedback

cmslewis commented Jun 13, 2017 •

edited

Loading