-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hotkeys in SQL Lab #4680
Hotkeys in SQL Lab #4680
Conversation
mistercrunch
commented
Mar 23, 2018
•
edited
Loading
edited
Codecov Report
@@ Coverage Diff @@
## master #4680 +/- ##
==========================================
+ Coverage 71.4% 71.59% +0.19%
==========================================
Files 190 191 +1
Lines 14934 14950 +16
Branches 1102 1100 -2
==========================================
+ Hits 10663 10703 +40
+ Misses 4268 4244 -24
Partials 3 3
Continue to review full report at Codecov.
|
removed the [WiP] tag, this is ready for review |
@@ -226,6 +229,32 @@ class SqlEditor extends React.PureComponent { | |||
render() { | |||
const height = this.sqlEditorHeight(); | |||
const defaultNorthHeight = this.props.queryEditor.height || 200; | |||
const SQLLAB_HOTKEYS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we put this at the top of the file or even in config file for adding new hotkeys more easily. I'm like the idea of a having this separate and we import it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the actions aren't in scope at that point though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh true, the func
param, i just don't like the bulky render view, but if there isn't a way to clean it up we can just go with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to this.getHotkeyConfig()
@@ -196,7 +196,10 @@ export function setDatabases(databases) { | |||
} | |||
|
|||
export function addQueryEditor(queryEditor) { | |||
const newQe = Object.assign({}, queryEditor, { id: shortid.generate() }); | |||
const newQe = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: neQe >> newQuery
<Popover id="popover-hotkeys" title={this.props.header} style={{ width: '300px' }}> | ||
<Table | ||
className="table table-condensed" | ||
data={this.props.hotkeys.map(keyConfig => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!!!!
🚢 |
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments
* Hotkeys * Making it work in AceEditor * Addressing comments