-
-
Notifications
You must be signed in to change notification settings - Fork 278
Conversation
Thanks for the PR. I'll review it on Monday. @shannonlal I guess you may want to have a look at this PR, as this touches the UI. |
app/components/Layout.react.js
Outdated
@@ -0,0 +1,31 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
|
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.
Add comment with a description. Something like "this module contains a collection of helper components for layout purposes" would be enough, unless there is something about the usage of <Row>
and <Column>
that developers should know.
app/components/Modal.react.js
Outdated
const Modal = props => { | ||
const EnhancedClass = enhanceWithClickOutside(class extends Component { | ||
handleClickOutside() { | ||
// eslint-disable-next-line |
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.
remove // eslint-disable-next-line
app/components/Modal.react.js
Outdated
}); | ||
|
||
return props.open | ||
? ( |
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.
format ternary expressions as:
return props.open ? (
expressionIfTrue
) : (
expressionIfFalse
);
@@ -15,3 +15,8 @@ | |||
.react-resizable-handle { | |||
z-index: 9; | |||
} | |||
|
|||
/* TODO move this override */ | |||
.scheduler .CodeMirror { |
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.
remove this change (not needed for this PR)
import PropTypes from 'prop-types'; | ||
|
||
import ReactDataGrid from 'react-data-grid'; | ||
import ms from 'ms'; |
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.
add ms
to devDependencies
Falcon uses webpack
to bundle most of if its dependencies and for this reason most of them are listed as devDependencies
.
Only dependencies that webpack
is unable to bundle (e.g. node native modules) should be added as dependencies
.
import matchSorter from 'match-sorter'; | ||
import Highlight from 'react-highlight'; | ||
|
||
import { Link } from '../Link.react'; |
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.
use extensions, i.e. import { Link } from '../Link.react.js';
import Highlight from 'react-highlight'; | ||
|
||
import { Link } from '../Link.react'; | ||
import { Row, Column } from '../Layout.react'; |
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.
.js
|
||
import { Link } from '../Link.react'; | ||
import { Row, Column } from '../Layout.react'; | ||
import Modal from '../Modal.react'; |
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.
.js
import { Link } from '../Link.react'; | ||
import { Row, Column } from '../Layout.react'; | ||
import Modal from '../Modal.react'; | ||
import { plotlyUrl } from '../../utils/utils'; |
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.
.js
- Add comment - Remove old code - Add ms dev
|
||
class IntervalFormatter extends React.Component { | ||
static propTypes = { | ||
value: PropTypes.object |
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.
similarly here: either rename value
or add description.
}; | ||
const boxStyle = { boxSizing: 'border-box', width: '50%' }; | ||
|
||
const MetaPreview = props => { |
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.
I'm not done writing the review but I've run out of time today. I'll continue tomorrow. Here are some general code style changes (for new code):
To discuss:
|
@@ -327,6 +335,7 @@ class Settings extends Component { | |||
<TabList> | |||
<Tab>Connection</Tab> | |||
<Tab disabled={queryPanelDisabled}>Query</Tab> | |||
<Tab disabled={false}>Schedule</Tab> |
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.
disabled={false}
package.json
Outdated
@@ -237,16 +238,20 @@ | |||
"dtrace-provider": "^0.8.6", | |||
"font-awesome": "^4.6.1", | |||
"ibm_db": "^2.3.0", | |||
"match-sorter": "^2.2.3", |
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.
move to devDependencies
package.json
Outdated
"mysql": "^2.15.0", | ||
"oracledb": "https://github.com/oracle/node-oracledb/releases/download/v2.2.0/oracledb-src-2.2.0.tgz", | ||
"papaparse": "^4.3.7", | ||
"pg": "^4.5.5", | ||
"pg-hstore": "^2.3.2", | ||
"react-click-outside": "^3.0.1", |
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.
move to devDependencies
package.json
Outdated
"mysql": "^2.15.0", | ||
"oracledb": "https://github.com/oracle/node-oracledb/releases/download/v2.2.0/oracledb-src-2.2.0.tgz", | ||
"papaparse": "^4.3.7", | ||
"pg": "^4.5.5", | ||
"pg-hstore": "^2.3.2", | ||
"react-click-outside": "^3.0.1", | ||
"react-highlight": "^0.12.0", |
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.
move to devDependencies
package.json
Outdated
"restify": "^4.3.2", | ||
"sequelize": "^3.30.4", | ||
"source-map-support": "^0.5.0", | ||
"sqlite3": "^3.1.13", | ||
"tedious": "^2.1.4" | ||
"tedious": "^2.1.4", | ||
"tohash": "^1.0.2" |
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.
move to devDependencies
query: PropTypes.object | ||
}; | ||
|
||
export const MetaPreviewModal = props => ( |
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.
Is there any need for having two components: MetaPreview
and MetaPreviewModal
?
If possible, I'd like to have a single component with a name that doesn't require to document the purpose of these components (how about SchedulePreview
?).
TODO
|
…/components/Settings/scheduler.test.jsx
@n-riesco we've made all the fixes as per your latest feedback |
I've tested it locally and it looks good. I'll merge it as soon as the tests in CircleCI succeed. @briandennis Thanks! It's been a pleasure to read such a clean PR. |
Adds a tab to display the existing scheduled queries in a table. The queries can be searched and when clicked, display the full query string as well as a link to the grid.
Note, this first step only allows query viewing. Creation, deletion, and updating will come in later PRs.