From 27d4b0a755e4fb1cc31e68d4cbacb3e02037798d Mon Sep 17 00:00:00 2001
From: Florian Scholdei <florian.scholdei@cloudogu.com>
Date: Tue, 20 Oct 2020 10:18:44 +0200
Subject: [PATCH] Fix review findings

---
 .../src/buttons/OpenInFullscreenButton.tsx    | 23 +++++++---
 scm-ui/ui-components/src/repos/DiffFile.tsx   | 40 ++++-------------
 .../components/content/SourcecodeViewer.tsx   | 23 ++++------
 .../src/repos/sources/containers/Content.tsx  | 44 +++++--------------
 4 files changed, 44 insertions(+), 86 deletions(-)

diff --git a/scm-ui/ui-components/src/buttons/OpenInFullscreenButton.tsx b/scm-ui/ui-components/src/buttons/OpenInFullscreenButton.tsx
index c1b4cab65e..afe16908f6 100644
--- a/scm-ui/ui-components/src/buttons/OpenInFullscreenButton.tsx
+++ b/scm-ui/ui-components/src/buttons/OpenInFullscreenButton.tsx
@@ -22,12 +22,14 @@
  * SOFTWARE.
  */
 import * as React from "react";
-import { FC } from "react";
+import { FC, ReactNode, useState } from "react";
 import { useTranslation } from "react-i18next";
 import styled from "styled-components";
+import FullscreenModal from "../modals/FullscreenModal";
 
 type Props = {
-  onClick?: () => void;
+  modalTitle: string;
+  modalBody: ReactNode;
 };
 
 const Button = styled.a`
@@ -37,13 +39,22 @@ const Button = styled.a`
   }
 `;
 
-const OpenInFullscreenButton: FC<Props> = ({ onClick }) => {
+const OpenInFullscreenButton: FC<Props> = ({ modalTitle, modalBody }) => {
   const [t] = useTranslation("repos");
+  const [showModal, setShowModal] = useState(false);
 
   return (
-    <Button title={t("diff.fullscreen.open")} className="button" onClick={onClick}>
-      <i className="fas fa-search-plus" />
-    </Button>
+    <>
+      <Button title={t("diff.fullscreen.open")} className="button" onClick={() => setShowModal(true)}>
+        <i className="fas fa-search-plus" />
+      </Button>
+      <FullscreenModal
+        title={modalTitle}
+        closeFunction={() => setShowModal(false)}
+        body={modalBody}
+        active={showModal}
+      />
+    </>
   );
 };
 
diff --git a/scm-ui/ui-components/src/repos/DiffFile.tsx b/scm-ui/ui-components/src/repos/DiffFile.tsx
index d8fee267df..77d44ec595 100644
--- a/scm-ui/ui-components/src/repos/DiffFile.tsx
+++ b/scm-ui/ui-components/src/repos/DiffFile.tsx
@@ -33,7 +33,7 @@ import Icon from "../Icon";
 import { Change, ChangeEvent, DiffObjectProps, File, Hunk as HunkType } from "./DiffTypes";
 import TokenizedDiffView from "./TokenizedDiffView";
 import DiffButton from "./DiffButton";
-import { MenuContext, OpenInFullscreenButton, FullscreenModal } from "@scm-manager/ui-components";
+import { MenuContext, OpenInFullscreenButton } from "@scm-manager/ui-components";
 import DiffExpander, { ExpandableHunk } from "./DiffExpander";
 import HunkExpandLink from "./HunkExpandLink";
 import { Modal } from "../modals";
@@ -57,7 +57,6 @@ type State = Collapsible & {
   sideBySide?: boolean;
   diffExpander: DiffExpander;
   expansionError?: any;
-  showFullscreenModal: boolean;
 };
 
 const DiffFilePanel = styled.div`
@@ -108,8 +107,7 @@ class DiffFile extends React.Component<Props, State> {
       collapsed: this.defaultCollapse(),
       sideBySide: props.sideBySide,
       diffExpander: new DiffExpander(props.file),
-      file: props.file,
-      showFullscreenModal: false
+      file: props.file
     };
   }
 
@@ -150,18 +148,6 @@ class DiffFile extends React.Component<Props, State> {
     );
   };
 
-  openModal = () => {
-    this.setState({
-      showFullscreenModal: true
-    });
-  };
-
-  closeModal = () => {
-    this.setState({
-      showFullscreenModal: false
-    });
-  };
-
   setCollapse = (collapsed: boolean) => {
     this.setState({
       collapsed
@@ -421,7 +407,7 @@ class DiffFile extends React.Component<Props, State> {
 
   render() {
     const { fileControlFactory, fileAnnotationFactory, t } = this.props;
-    const { file, collapsed, sideBySide, diffExpander, expansionError, showFullscreenModal } = this.state;
+    const { file, collapsed, sideBySide, diffExpander, expansionError } = this.state;
     const viewType = sideBySide ? "split" : "unified";
 
     let body = null;
@@ -444,7 +430,12 @@ class DiffFile extends React.Component<Props, State> {
     }
     const collapseIcon = this.hasContent(file) ? <Icon name={icon} color="inherit" /> : null;
     const fileControls = fileControlFactory ? fileControlFactory(file, this.setCollapse) : null;
-    const openInFullscreen = file?.hunks?.length ? <OpenInFullscreenButton onClick={this.openModal} /> : null;
+    const openInFullscreen = file?.hunks?.length ? (
+      <OpenInFullscreenButton
+        modalTitle={file.type === "delete" ? file.oldPath : file.newPath}
+        modalBody={<MarginlessModalContent>{body}</MarginlessModalContent>}
+      />
+    ) : null;
     const sideBySideToggle = file?.hunks?.length && (
       <MenuContext.Consumer>
         {({ setCollapsed }) => (
@@ -484,18 +475,6 @@ class DiffFile extends React.Component<Props, State> {
       );
     }
 
-    let modalContent;
-    if (file?.hunks?.length) {
-      modalContent = (
-        <FullscreenModal
-          title={file.type === "delete" ? file.oldPath : file.newPath}
-          closeFunction={() => this.closeModal()}
-          body={<MarginlessModalContent>{body}</MarginlessModalContent>}
-          active={showFullscreenModal}
-        />
-      );
-    }
-
     return (
       <DiffFilePanel
         className={classNames("panel", "is-size-6")}
@@ -520,7 +499,6 @@ class DiffFile extends React.Component<Props, State> {
           </FlexWrapLevel>
         </div>
         {body}
-        {modalContent}
       </DiffFilePanel>
     );
   }
diff --git a/scm-ui/ui-webapp/src/repos/sources/components/content/SourcecodeViewer.tsx b/scm-ui/ui-webapp/src/repos/sources/components/content/SourcecodeViewer.tsx
index 080db32b2d..2eae04c8a6 100644
--- a/scm-ui/ui-webapp/src/repos/sources/components/content/SourcecodeViewer.tsx
+++ b/scm-ui/ui-webapp/src/repos/sources/components/content/SourcecodeViewer.tsx
@@ -37,32 +37,25 @@ const SourcecodeViewer: FC<Props> = ({ file, language }) => {
   const [currentFileRevision, setCurrentFileRevision] = useState("");
 
   useEffect(() => {
-    function fetchContent() {
+    if (file.revision !== currentFileRevision) {
       getContent((file._links.self as Link).href)
         .then(content => {
-          setLoading(false);
           setContent(content);
           setCurrentFileRevision(file.revision);
-        })
-        .catch(error => {
           setLoading(false);
-          setError(error);
-        });
+        })
+        .catch(setError);
     }
-
-    if (file.revision !== currentFileRevision) {
-      fetchContent();
-    }
-  });
-
-  if (loading) {
-    return <Loading />;
-  }
+  }, [currentFileRevision, file]);
 
   if (error) {
     return <ErrorNotification error={error} />;
   }
 
+  if (loading) {
+    return <Loading />;
+  }
+
   if (!content) {
     return null;
   }
diff --git a/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx b/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx
index 7a2af3e2fa..7f827f855a 100644
--- a/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx
+++ b/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx
@@ -21,21 +21,14 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-import React from "react";
+import React, { ReactNode } from "react";
 import { connect } from "react-redux";
 import { WithTranslation, withTranslation } from "react-i18next";
 import classNames from "classnames";
 import styled from "styled-components";
 import { ExtensionPoint } from "@scm-manager/ui-extensions";
 import { File, Repository } from "@scm-manager/ui-types";
-import {
-  DateFromNow,
-  ErrorNotification,
-  FileSize,
-  Icon,
-  OpenInFullscreenButton,
-  FullscreenModal
-} from "@scm-manager/ui-components";
+import { DateFromNow, ErrorNotification, FileSize, Icon, OpenInFullscreenButton } from "@scm-manager/ui-components";
 import { getSources } from "../modules/sources";
 import FileButtonAddons from "../components/content/FileButtonAddons";
 import SourcesView from "./SourcesView";
@@ -55,7 +48,6 @@ type State = {
   collapsed: boolean;
   selected: SourceViewSelection;
   errorFromExtension?: Error;
-  showFullscreenModal: boolean;
 };
 
 const Header = styled.div`
@@ -104,8 +96,7 @@ class Content extends React.Component<Props, State> {
 
     this.state = {
       collapsed: true,
-      selected: "source",
-      showFullscreenModal: false
+      selected: "source"
     };
   }
 
@@ -115,25 +106,13 @@ class Content extends React.Component<Props, State> {
     }));
   };
 
-  openModal = () => {
-    this.setState({
-      showFullscreenModal: true
-    });
-  };
-
-  closeModal = () => {
-    this.setState({
-      showFullscreenModal: false
-    });
-  };
-
   handleExtensionError = (error: Error) => {
     this.setState({
       errorFromExtension: error
     });
   };
 
-  showHeader() {
+  showHeader(content: ReactNode) {
     const { repository, file, revision } = this.props;
     const { selected, collapsed } = this.state;
     const icon = collapsed ? "angle-right" : "angle-down";
@@ -156,7 +135,10 @@ class Content extends React.Component<Props, State> {
           </div>
           <div className="buttons is-grouped">
             {selector}
-            <OpenInFullscreenButton onClick={this.openModal} />
+            <OpenInFullscreenButton
+              modalTitle={file?.name}
+              modalBody={<BorderLessDiv className="panel">{content}</BorderLessDiv>}
+            />
             <ExtensionPoint
               name="repos.sources.content.actionbar"
               props={{
@@ -237,9 +219,8 @@ class Content extends React.Component<Props, State> {
 
   render() {
     const { file, revision, repository, path, breadcrumb } = this.props;
-    const { selected, errorFromExtension, showFullscreenModal } = this.state;
+    const { selected, errorFromExtension } = this.state;
 
-    const header = this.showHeader();
     let content;
     switch (selected) {
       case "source":
@@ -251,6 +232,7 @@ class Content extends React.Component<Props, State> {
       case "annotations":
         content = <AnnotateView file={file} repository={repository} />;
     }
+    const header = this.showHeader(content);
     const moreInformation = this.showMoreInformation();
 
     return (
@@ -260,12 +242,6 @@ class Content extends React.Component<Props, State> {
           <Header>{header}</Header>
           {moreInformation}
           {content}
-          <FullscreenModal
-            title={file?.name}
-            closeFunction={() => this.closeModal()}
-            body={<BorderLessDiv className="panel">{content}</BorderLessDiv>}
-            active={showFullscreenModal}
-          />
         </div>
         {errorFromExtension && <ErrorNotification error={errorFromExtension} />}
       </div>