NEW styles and fixes for styling and code cleanliness

NEW Add pointer cursor for versions in the list on hover FIX Ensure toolbar--condensed class is correctly removed when component unmounts FIX Correct eslint violations and provide component in broken unit test
silverstripe · Aug 1, 2018 · a8e0d63 · a8e0d63
1 parent f3f6a56
commit a8e0d63
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 37 deletions.
diff --git a/client/src/components/HistoryViewer/HistoryViewer.js b/client/src/components/HistoryViewer/HistoryViewer.js
@@ -158,7 +158,9 @@ class HistoryViewer extends Component {
 
     const filterVersions = (wantedID) => (potential => potential.Version === wantedID);
 
-    const version = this.getVersions().find(filterVersions(currentVersion));
+    const version = this.getVersions().find(
+      filterVersions(compareMode ? compareFrom : currentVersion)
+    );
     const latestVersion = this.getLatestVersion();
     const compare = compareMode ? {
       versionFrom: this.getVersions().find(filterVersions(compareFrom)),

diff --git a/client/src/components/HistoryViewer/HistoryViewer.scss b/client/src/components/HistoryViewer/HistoryViewer.scss
@@ -8,6 +8,10 @@
   }
 }
 
+.history-viewer__table:not(.history-viewer__table--current) tbody tr:hover {
+  cursor: pointer;
+}
+
 // Used in conjunction with .panel--padded, removes top and bottom padding
 .panel--padded-side {
   padding-bottom: 0;

diff --git a/client/src/components/HistoryViewer/HistoryViewerVersionDetail.js b/client/src/components/HistoryViewer/HistoryViewerVersionDetail.js
@@ -8,11 +8,11 @@ import { versionType } from 'types/versionType';
 
 class HistoryViewerVersionDetail extends PureComponent {
   componentWillMount() {
-    this.toggleToolbarClass();
+    this.toggleToolbarClass(true);
   }
 
   componentWillUnmount() {
-    this.toggleToolbarClass();
+    this.toggleToolbarClass(false);
   }
 
   /**
@@ -52,15 +52,20 @@ class HistoryViewerVersionDetail extends PureComponent {
   /**
    * Until the CMS is fully React driven, we must control certain aspects of the CMS DOM with
    * manual CSS tweaks. @todo remove this when React drives the CMS.
+   *
+   * @param {boolean} add
    */
-  toggleToolbarClass() {
+  toggleToolbarClass(add = true) {
     const selector = document
       .querySelector('.CMSPageHistoryViewerController div:not(.cms-content-tools) .cms-content-header');
+    const className = 'history-viewer__toolbar--condensed';
 
     if (selector && this.isPreviewable()) {
-      selector
-        .classList
-        .toggle('history-viewer__toolbar--condensed');
+      if (add) {
+        selector.classList.add(className);
+      } else {
+        selector.classList.remove(className);
+      }
     }
   }
 
@@ -99,9 +104,11 @@ class HistoryViewerVersionDetail extends PureComponent {
    */
   renderToolbar() {
     const { ToolbarComponent, isLatestVersion, recordId, version } = this.props;
+
     if (this.isCompareMode()) {
       return null;
     }
+
     return (
       <ToolbarComponent
         identifier="HistoryViewer.VersionDetail.Toolbar"

diff --git a/client/src/components/HistoryViewer/HistoryViewerVersionList.js b/client/src/components/HistoryViewer/HistoryViewerVersionList.js
@@ -18,6 +18,23 @@ class HistoryViewerVersionList extends PureComponent {
     return classnames({ table: true }, extraClass);
   }
 
+  /**
+   * "isActive" in this component indicates that the content is shown - ie. the table
+   * only shows the row (or rows) that are currently highlighted above the content of
+   * this version.
+   *
+   * @param {Object} version
+   * @returns {boolean}
+   */
+  isVersionActive(version) {
+    const { isActive, compareFrom, compareTo } = this.props;
+    if (isActive) {
+      return true;
+    }
+
+    return version.Version === compareFrom || version.Version === compareTo;
+  }
+
   /**
    * Render any messages into the form
    *
@@ -85,7 +102,7 @@ HistoryViewerVersionList.propTypes = {
 };
 
 HistoryViewerVersionList.defaultProps = {
-  extraClass: 'table-hover',
+  extraClass: 'table-hover history-viewer__table',
   isActive: false,
   messages: [],
   versions: [],

diff --git a/client/src/components/HistoryViewer/tests/HistoryViewer-test.js b/client/src/components/HistoryViewer/tests/HistoryViewer-test.js
@@ -1,17 +1,19 @@
+/* eslint-disable import/no-extraneous-dependencies */
 /* global jest, describe, it, expect */
 
 import React from 'react';
 import ReactTestUtils from 'react-addons-test-utils';
 import { Component as HistoryViewer } from '../HistoryViewer';
-import Enzyme, { shallow } from 'enzyme';
-import Adapter from "enzyme-adapter-react-15.4/build/index";
+import Enzyme from 'enzyme';
+import Adapter from 'enzyme-adapter-react-15.4/build/index';
 
 Enzyme.configure({ adapter: new Adapter() });
 
 describe('HistoryViewer', () => {
   let component = null;
   const ListComponent = () => <table />;
   const VersionDetailComponent = () => <div />;
+  const CompareWarningComponent = () => <div />;
 
   const versions = {
     Versions: {
@@ -57,6 +59,7 @@ describe('HistoryViewer', () => {
         <HistoryViewer
           ListComponent={ListComponent}
           VersionDetailComponent={VersionDetailComponent}
+          CompareWarningComponent={CompareWarningComponent}
           versions={versions}
           recordId={1}
           limit={100}
@@ -73,6 +76,7 @@ describe('HistoryViewer', () => {
         <HistoryViewer
           ListComponent={ListComponent}
           VersionDetailComponent={VersionDetailComponent}
+          CompareWarningComponent={CompareWarningComponent}
           versions={versions}
           recordId={1}
           limit={100}
@@ -87,6 +91,7 @@ describe('HistoryViewer', () => {
         <HistoryViewer
           ListComponent={ListComponent}
           VersionDetailComponent={VersionDetailComponent}
+          CompareWarningComponent={CompareWarningComponent}
           versions={versions}
           recordId={1}
           limit={100}
@@ -103,6 +108,7 @@ describe('HistoryViewer', () => {
         <HistoryViewer
           ListComponent={ListComponent}
           VersionDetailComponent={VersionDetailComponent}
+          CompareWarningComponent={CompareWarningComponent}
           versions={versions}
           recordId={1}
           limit={100}

diff --git a/client/src/components/HistoryViewer/tests/HistoryViewerHeading-test.js b/client/src/components/HistoryViewer/tests/HistoryViewerHeading-test.js
@@ -1,7 +1,7 @@
+/* eslint-disable import/no-extraneous-dependencies */
 /* global jest, describe, it, expect */
 
 import React from 'react';
-import ReactTestUtils from 'react-addons-test-utils';
 import { Component as HistoryViewerHeading } from '../HistoryViewerHeading';
 import Enzyme, { shallow } from 'enzyme';
 import Adapter from 'enzyme-adapter-react-15.4/build/index';
@@ -46,31 +46,5 @@ describe('HistoryViewerHeading', () => {
       });
     });
   });
-
-
-  class HeadingWrapper extends React.PureComponent {
-    render() {
-      return (
-        <table>
-          <thead>
-            <HistoryViewerHeading {...this.props} />
-          </thead>
-        </table>
-      );
-    }
-  }
-
-  it('has four columns when hasActions is true', () => {
-    const component = ReactTestUtils.renderIntoDocument(
-      <HeadingWrapper hasActions />
-    );
-
-    const result = ReactTestUtils.scryRenderedDOMComponentsWithTag(
-      component,
-      'th'
-    );
-
-    expect(result.length).toBe(4);
-  });
 });