Some polish on query search #1222
Conversation
| @@ -42,6 +52,11 @@ class QueryTable extends React.Component { | |||
| if (q.endDttm) { | |||
| q.duration = fDuration(q.startDttm, q.endDttm); | |||
| } | |||
| q.userId = ( | |||
| <a onClick={this.props.onUserClicked.bind(this, q.userId)}> | |||
There was a problem hiding this comment.
when we don't have an href we should use a button with a link class:
<button class="btn btn-link" onClick={}>Link</button>
|
|
||
| q.querylink = ( | ||
| <div style={{ width: '75px' }}> | ||
| <a href={this.getQueryLink(q.dbId, source)} > |
There was a problem hiding this comment.
could we use a mini button here with the icon inside?
<a href="#" class="btn btn-primary btn-xs"><i class="fa fa-external-link"></i> Open in SQL Editor</a>
| @@ -98,7 +113,13 @@ class QueryTable extends React.Component { | |||
| /> | |||
| </div> | |||
| ); | |||
|
|
|||
| q.querylink = ( | |||
| <div style={{ width: '75px' }}> | |||
There was a problem hiding this comment.
i think we should let the button be full-width/on one line, and make the sql-query container a little less wide.
| showVisualizeModal: false, | ||
| activeQuery: null, | ||
| }; | ||
| } | ||
| getQueryLink(dbId, sql) { |
There was a problem hiding this comment.
i think we are using a similar getQueryLink method in a few components. can we generalize and add to a utils file?
vera-liu
force-pushed
the
vliu_search_polish
branch
from
037615c
to
06e05e6
Compare
|
Made some changes, also made dbId clickable like userId does: |
vera-liu
force-pushed
the
vliu_search_polish
branch
from
06e05e6
to
1439ead
Compare
| @@ -46,6 +43,12 @@ class DatabaseSelect extends React.Component { | |||
|
|
|||
| DatabaseSelect.propTypes = { | |||
| onChange: React.PropTypes.func, | |||
| databaseId: React.PropTypes.number, | |||
There was a problem hiding this comment.
i think this will probably through a warning when the default prop is set to null and it's expecting a number.
probably want to use React.PropTypes.oneOfType
React.PropTypes.oneOfType([
React.PropTypes.number,
React.PropTypes.bool
]);and make databaseId: false in the defaultProps
and databaseId
There was a problem hiding this comment.
I tested this passing databaseId as null, no warnings were shown. According to https://github.com/facebook/react/issues/2166, for proptypes that are not isRequired, the null value means it's intentional, which will be bypassed by React, the undefined value will cause a warning. For this one I think we don't have to put databaseId as isRequired since it's for passing the value to Select. If null is passed then the Select box is empty, as in default state. What are your suggestions? @ascott
There was a problem hiding this comment.
if it doesn't throw a warning, i'm good with your implementation 👍
- Changed query search icon - CopyToClipboard in action bar
vera-liu
force-pushed
the
vliu_search_polish
branch
from
57068b3
to
ee91ecd
Compare
…#1222) * fix(plugin-chart-table): fix table clear x-filtering highlighting issue * fix(plugin-chart-table): fix lint
…#1222) * fix(plugin-chart-table): fix table clear x-filtering highlighting issue * fix(plugin-chart-table): fix lint
…#1222) * fix(plugin-chart-table): fix table clear x-filtering highlighting issue * fix(plugin-chart-table): fix lint
…#1222) * fix(plugin-chart-table): fix table clear x-filtering highlighting issue * fix(plugin-chart-table): fix lint
mistercrunch
added
🏷️ bot
Done:
Todo:
needs-review @ascott @bkyryliuk @mistercrunch