[GitHub] [incubator-superset] ktmud commented on a change in pull request #9886: feat: [dashboard] notification and warning for auto force refresh

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org