Skip to content
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

[sql lab] only show single run query button #1858

Merged
merged 4 commits into from
Jan 4, 2017

Conversation

ascott
Copy link

@ascott ascott commented Dec 16, 2016

  • show single run button rather than Run Query and Run Async Query
  • if async is possible, run async, otherwise run sync
  • move buttons to their own component

fixes this issue: #1819

before
screenshot 2016-12-16 13 40 52
screenshot 2016-12-16 13 41 03

after
screenshot 2016-12-16 13 28 25
screenshot 2016-12-16 13 27 54
run-selected

plz review @mistercrunch @vera-liu @bkyryliuk

@ascott ascott added the sqllab Namespace | Anything related to the SQL Lab label Dec 16, 2016
@@ -208,7 +149,13 @@ class SqlEditor extends React.PureComponent {
<div className="sql-toolbar clearfix">
<div className="pull-left">
<Form inline>
{runButtons}
<RunQueryActionButton
queryEditor={this.props.queryEditor}
Copy link
Member

Choose a reason for hiding this comment

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

Curious to hear your thoughts on passing larger objects or more specific props. For instance here we could define a isTextSelected prop instead of passing queryEditor. Similarly we could pass allowAsync instead of database.

Isn't it better to pass the more targeted props? It probably means less re-rendering.

I guess it depends on how the component may or may not evolve (say to use other attributes of that object), and whether we'd want to use the component outside of the context of the application / object definition.

Copy link
Author

Choose a reason for hiding this comment

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

this is a good point. i agree passing in just the props the component needs is ideal. i'll make this change.

@ascott ascott force-pushed the alanna-default-to-async branch from b1d312a to 0081e6f Compare January 3, 2017 21:16
@ascott ascott force-pushed the alanna-default-to-async branch 2 times, most recently from b52e6a5 to cef62f2 Compare January 4, 2017 00:48
@ascott ascott force-pushed the alanna-default-to-async branch from cef62f2 to 12eee9f Compare January 4, 2017 00:52
@ascott ascott merged commit 242869d into apache:master Jan 4, 2017
@ascott ascott deleted the alanna-default-to-async branch January 4, 2017 01:20
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.15.2 labels Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels sqllab Namespace | Anything related to the SQL Lab 🚢 0.15.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants