ktmud commented on a change in pull request #9886:
URL:
https://github.com/apache/incubator-superset/pull/9886#discussion_r434180763
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -86,6 +89,7 @@ const propTypes = {
setMaxUndoHistoryExceeded: PropTypes.func.isRequired,
maxUndoHistoryToast: PropTypes.func.isRequired,
refreshFrequency: PropTypes.number.isRequired,
+ isPersistentRefreshFrequency: PropTypes.bool.isRequired,
Review comment:
Nit: maybe name this `shouldPersistRefreshFrequency`, but up to you.
##########
File path: superset-frontend/src/dashboard/components/RefreshIntervalModal.jsx
##########
@@ -46,23 +54,43 @@ const options = [
class RefreshIntervalModal extends React.PureComponent {
constructor(props) {
super(props);
+ this.modal = React.createRef();
this.state = {
refreshFrequency: props.refreshFrequency,
};
this.handleFrequencyChange = this.handleFrequencyChange.bind(this);
+ this.onSave = this.onSave.bind(this);
+ this.onCancel = this.onCancel.bind(this);
+ }
+
+ onSave() {
+ this.props.onChange(this.state.refreshFrequency, this.props.editMode);
+ this.modal.current.close();
+ }
+
+ onCancel() {
+ this.setState({
+ refreshFrequency: this.props.refreshFrequency,
+ });
+ this.modal.current.close();
}
handleFrequencyChange(opt) {
const value = opt ? opt.value : options[0].value;
this.setState({
refreshFrequency: value,
});
- this.props.onChange(value, this.props.editMode);
}
render() {
+ const { refreshLimit = 0, refreshWarning, editMode } = this.props;
+ const { refreshFrequency = 0 } = this.state;
+ const showRefreshWarning =
+ !!refreshFrequency && !!refreshWarning && refreshFrequency <
refreshLimit;
Review comment:
Is `!!refreshFrequency` and `!!refreshWarning` necessary? `<` will
always be false if either side is `undefined`.
##########
File path: superset-frontend/src/dashboard/components/Header.jsx
##########
@@ -249,14 +272,21 @@ class Header extends React.PureComponent {
colorNamespace,
colorScheme,
dashboardInfo,
- refreshFrequency,
+ refreshFrequency: currentRefreshFrequency,
+ isPersistentRefreshFrequency,
} = this.props;
const scale = CategoricalColorNamespace.getScale(
colorScheme,
colorNamespace,
);
const labelColors = colorScheme ? scale.getColorMap() : {};
+ // check refresh frequency is for current session or persist
+ const refreshFrequency = getPersistentRefreshFrequency({
+ currentRefreshFrequency,
+ isPersistent: isPersistentRefreshFrequency,
+ dashboardInfo,
+ });
Review comment:
I don't think we need a util function for this. It might be more
straightforward to simply do
```js
const refreshFrequency = isPersistentRefreshFrequency
? currentRefreshFrequency
: dashboardInto.metadata.refresh_frequency;
```
##########
File path: superset-frontend/src/dashboard/components/RefreshIntervalModal.jsx
##########
@@ -46,23 +54,43 @@ const options = [
class RefreshIntervalModal extends React.PureComponent {
constructor(props) {
super(props);
+ this.modal = React.createRef();
Review comment:
Nit: name this `modalRef` to differentiate with directly references
assigned by ref functions:
```jsx
this.modal = null
<Modal ref={current => { this.modal = current }}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
