[explore] refactor slice action button group #1074
Conversation
|
|
||
| beforeOpen() { | ||
| this.setState({ | ||
| viewSqlQuery: this.props.slice.viewSqlQuery, |
There was a problem hiding this comment.
this.props.slice gets updated after everything is mounted, so before we open the modal we need to set the state with the updated slice data. i should add a todo, to fix this once caravel.js is refactored to better work with the react structure/lifecycle.
| import React, { PropTypes } from 'react'; | ||
| import { Button, Tooltip, OverlayTrigger } from 'react-bootstrap'; | ||
|
|
||
| const propTypes = { |
There was a problem hiding this comment.
I need to replicate this pattern of setting the propTypes & defaultTypes at the top of the file in SqlLab!
| }; | ||
|
|
||
| // bindings | ||
| this.copyToClipboard = this.copyToClipboard.bind(this); |
There was a problem hiding this comment.
sucks that we need to write this boilerplate, but haven't the .bind in render isn't better...
There was a problem hiding this comment.
yeah, i like to keep it all in one place in the constructor.
| } | ||
|
|
||
| tooltipText() { | ||
| let tooltipText; |
There was a problem hiding this comment.
optional: ternary operator could be nice here, the whole function becomes a single line
|
|
||
| render() { | ||
| return ( | ||
| <span |
There was a problem hiding this comment.
this component is great but on my side I wanted to use it not as a button (just an <i/>). Maybe instead of receiving buttonBody is could receive trigger as any jsx, but still be wrapped in an <span onClick>.
Another option is to put this.props.children inside the onClick span as opposed to this.props.trigger.
There was a problem hiding this comment.
i updated this so you can just pass in <i />, also added a boolean prop isButton if you want it to look like a button. otherwise, it will wrap your triggerNode in a simple <a/>.
|
|
||
| render() { | ||
| return ( | ||
| <span |
There was a problem hiding this comment.
agreed, let's call it triggerNode and take out the btn styling so it can be used more generally.
|
LGTM |
mistercrunch
added
🏷️ bot
todo:
plz review @mistercrunch @bkyryliuk @vera-liu