Fix history list to load last in the stack

Fix moved loading overlay to the Gallery only, rather than the entire screen
silverstripe · Jul 31, 2017 · cb7d157 · cb7d157
1 parent 9ba9ff0
commit cb7d157
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 74 deletions.
diff --git a/client/dist/js/bundle.js b/client/dist/js/bundle.js
diff --git a/client/src/containers/AssetAdmin/AssetAdmin.js b/client/src/containers/AssetAdmin/AssetAdmin.js
@@ -596,10 +596,6 @@ class AssetAdmin extends SilverStripeComponent {
           {this.renderGallery()}
           {this.renderEditor()}
         </div>
-        {this.props.loading &&
-        [<div key="overlay" className="cms-content-loading-overlay ui-widget-overlay-light"></div>,
-        <div key="spinner" className="cms-content-loading-spinner"></div>]
-        }
       </div>
     );
   }

diff --git a/client/src/containers/Gallery/Gallery.js b/client/src/containers/Gallery/Gallery.js
@@ -100,8 +100,6 @@ class Gallery extends Component {
     if (!this.compareFiles(this.props.files, nextProps.files)) {
       nextProps.actions.queuedFiles.purgeUploadQueue();
     }
-
-    this.checkLoadingIndicator(nextProps);
   }
 
   componentDidUpdate() {
@@ -190,26 +188,6 @@ class Gallery extends Component {
     );
   }
 
-  /**
-   * Required anti-pattern, because `.cms-content` is the container for the React component.
-   *
-   * Adds or removes the load class from `.cms-content` if it is for the AssetAdmin
-   *
-   * @param {Object} props
-   */
-  checkLoadingIndicator(props) {
-    const $sectionWrapper = $('.cms-content.AssetAdmin');
-    if (!$sectionWrapper.length) {
-      return;
-    }
-
-    if (props.loading) {
-      $sectionWrapper.addClass('loading');
-    } else {
-      $sectionWrapper.removeClass('loading');
-    }
-  }
-
   /**
    * Compare two lists to see if equal
    *
@@ -844,7 +822,12 @@ class Gallery extends Component {
         );
       }
       if (this.props.loading) {
-        return <div className="flexbox-area-grow"></div>;
+        return (
+          <div className="flexbox-area-grow">
+            <div key="overlay" className="cms-content-loading-overlay ui-widget-overlay-light"></div>
+            <div key="spinner" className="cms-content-loading-spinner"></div>
+          </div>
+        );
       }
       return (
         <div className="flexbox-area-grow">
@@ -918,6 +901,10 @@ class Gallery extends Component {
             {this.renderGalleryView()}
           </GalleryDND>
         </AssetDropzone>
+        { this.props.loading && [
+          <div key="overlay" className="cms-content-loading-overlay ui-widget-overlay-light"></div>,
+          <div key="spinner" className="cms-content-loading-spinner"></div>,
+        ]}
       </div>
     );
   }

diff --git a/client/src/containers/HistoryList/HistoryList.js b/client/src/containers/HistoryList/HistoryList.js
@@ -7,6 +7,23 @@ import FormBuilderLoader from 'containers/FormBuilderLoader/FormBuilderLoader';
 
 const sectionConfigKey = 'SilverStripe\\AssetAdmin\\Controller\\AssetAdmin';
 
+/**
+ * Create a new endpoint
+ *
+ * @todo duplication with assetadmin.
+ *
+ * @param {Object} endpointConfig
+ * @param {Boolean} includeToken
+ * @returns {Function}
+ */
+const createEndpoint = (endpointConfig, includeToken = true) => (
+  backend.createEndpointFetcher(Object.assign(
+    {},
+    endpointConfig,
+    includeToken ? { defaultData: { SecurityID: Config.get('SecurityID') } } : {}
+  ))
+);
+
 class HistoryList extends Component {
 
   constructor(props) {
@@ -15,14 +32,15 @@ class HistoryList extends Component {
     this.state = {
       detailView: null,
       history: [],
-      /* TODO loading */
-      loadedDetails: true,
+      loadedDetails: false,
     };
 
     this.handleClick = this.handleClick.bind(this);
     this.handleBack = this.handleBack.bind(this);
 
-    this.api = this.createEndpoint(props.sectionConfig.historyEndpoint);
+    this.timer = null;
+
+    this.api = createEndpoint(props.sectionConfig.historyEndpoint);
   }
 
   componentDidMount() {
@@ -34,13 +52,8 @@ class HistoryList extends Component {
     this.refreshHistoryIfNeeded(nextProps);
   }
 
-  /**
-   * @returns {string} class
-   */
-  getContainerClassName() {
-    return (this.state.viewDetails && !this.state.loadedDetails)
-      ? 'history-list history-container--loading'
-      : 'history-list';
+  componentWillUnmount() {
+    clearTimeout(this.timer);
   }
 
   /**
@@ -49,15 +62,28 @@ class HistoryList extends Component {
    * @param {object} nextProps
    */
   refreshHistoryIfNeeded(nextProps) {
-    if (!nextProps
+    if (
+      (!nextProps && !this.state.loadedDetails)
       || (nextProps.data.fileId !== this.props.data.fileId)
       || (nextProps.data.latestVersionId !== this.props.data.latestVersionId)
     ) {
-      this.api({
-        fileId: (nextProps) ? nextProps.data.fileId : this.props.data.fileId,
-      }).then((history) => {
-        this.setState({ history });
-      });
+      this.setState({ loadedDetails: false });
+      const fileId = (nextProps) ? nextProps.data.fileId : this.props.data.fileId;
+      clearTimeout(this.timer);
+
+      /*
+       * This needs a delay/throttle, so this api request tries to be made last in the stack.
+       * We also use this to stop an API call happening if the component is going to
+       * unmount soon.
+       * TODO: This could potentially be solved by using apollo-client's caching and graphql.
+       */
+      this.timer = setTimeout(() => {
+        this.api({
+          fileId,
+        }).then((history) => {
+          this.setState({ history, loadedDetails: true });
+        });
+      }, 250);
     }
   }
 
@@ -83,57 +109,44 @@ class HistoryList extends Component {
     });
   }
 
-  /**
-   * Create a new endpoint
-   *
-   * @todo duplication with assetadmin.
-   *
-   * @param {Object} endpointConfig
-   * @param {Boolean} includeToken
-   * @returns {Function}
-   */
-  createEndpoint(endpointConfig, includeToken = true) {
-    return backend.createEndpointFetcher(Object.assign(
-      {},
-      endpointConfig,
-      includeToken ? { defaultData: { SecurityID: Config.get('SecurityID') } } : {}
-    ));
-  }
-
   render() {
-    const containerClassName = this.getContainerClassName();
-    if (!this.state.history) {
+    if (!this.state.loadedDetails) {
       return (
-        <div className={containerClassName} />
+        <div className="history-list history-list--loading">
+          Loading...
+        </div>
       );
     }
 
     if (this.state.viewDetails) {
-      let schemaUrl = [
+      const schemaUrl = [
         this.props.historySchemaUrl,
         this.props.data.fileId,
         this.state.viewDetails,
       ].join('/');
 
-      let className = [
-        'btn btn-secondary',
-        'btn--icon-xl btn--no-text',
+      const buttonClasses = [
+        'btn',
+        'btn-secondary',
+        'btn--icon-xl',
+        'btn--no-text',
         'font-icon-left-open-big',
         'history-list__back',
       ].join(' ');
 
       return (
-        <div className={containerClassName}>
-          <a className={className} onClick={this.handleBack} />
+        <div className="history-list">
+          <a className={buttonClasses} onClick={this.handleBack} />
           <FormBuilderLoader identifier="AssetAdmin.HistoryList" schemaUrl={schemaUrl} />
         </div>
       );
     }
 
+    const historyList = this.state.history || [];
     return (
-      <div className={containerClassName}>
+      <div className="history-list">
         <ul className="list-group list-group-flush history-list__list">
-          {this.state.history.map((history) => (
+          {historyList.map((history) => (
             <HistoryItem
               key={history.versionid}
               {...history}

diff --git a/code/Controller/AssetAdmin.php b/code/Controller/AssetAdmin.php
@@ -509,8 +509,8 @@ public function apiHistory(HTTPRequest $request)
             }
         }
 
-        return
-            (new HTTPResponse(json_encode($output)))->addHeader('Content-Type', 'application/json');
+        $response = new HTTPResponse(json_encode($output));
+        return $response->addHeader('Content-Type', 'application/json');
     }
 
     /**