Skip to content

implement "convert to boolean/number/string" in new data browser [axot: 71]#10734

Merged
davidsbailey merged 6 commits into
stagingfrom
convert-columns
Sep 20, 2016
Merged

implement "convert to boolean/number/string" in new data browser [axot: 71]#10734
davidsbailey merged 6 commits into
stagingfrom
convert-columns

Conversation

@davidsbailey

Copy link
Copy Markdown
Member

@davidsbailey davidsbailey changed the title implement "convert to boolean/number/string" in new data browser [axof: 71] implement "convert to boolean/number/string" in new data browser [axot: 71] Sep 20, 2016
<a onClick={() => this.coerceColumn('boolean')}>
Convert to boolean
</a>
</li>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i wonder if getDropdownMenu should instead just be its own react component?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By having these functions defined in the onClick, we'll create a new function (or a bunch of new functions) every time we re-render. It might be more performant to instead call a method on the class (i.e. onClick={this.coerceColumnString}) tho it's also quite likely that this is an unnecessary performance optimization.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, should we be using the ColumnType enum that gets defined elsewhere?

if (val === false || val === 'false') {
return false;
}
throw new Error('Unable to convert to boolean');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be worth a couple quick unit tests for these utils

<a onClick={() => this.coerceColumn('boolean')}>
Convert to boolean
</a>
</li>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, should we be using the ColumnType enum that gets defined elsewhere?

@davidsbailey davidsbailey merged commit 89e44a8 into staging Sep 20, 2016
@davidsbailey davidsbailey deleted the convert-columns branch September 20, 2016 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants