From 4b19d4dc9e0d5102cd4088531070a74f2e06973e Mon Sep 17 00:00:00 2001 From: Thomas Zerr Date: Thu, 19 Sep 2024 14:47:00 +0200 Subject: [PATCH] Fix jumping around in pull request diff view --- scm-ui/ui-components/src/index.ts | 2 +- scm-ui/ui-components/src/repos/Diff.tsx | 19 +++++++++-- .../ui-components/src/repos/LoadingDiff.tsx | 34 ++++--------------- .../ui-components/src/useScrollToElement.ts | 18 ++++++++-- 4 files changed, 39 insertions(+), 34 deletions(-) diff --git a/scm-ui/ui-components/src/index.ts b/scm-ui/ui-components/src/index.ts index 16c219cee5..b8fb9948ac 100644 --- a/scm-ui/ui-components/src/index.ts +++ b/scm-ui/ui-components/src/index.ts @@ -90,7 +90,7 @@ export * from "./devices"; export { default as copyToClipboard } from "./CopyToClipboard"; export { createA11yId } from "./createA11yId"; export { useSecondaryNavigation } from "./useSecondaryNavigation"; -export { default as useScrollToElement } from "./useScrollToElement"; +export { default as useScrollToElement, useScrollToElementWithCallback } from "./useScrollToElement"; export { default as DiffDropDown } from "./repos/DiffDropDown"; export { default as comparators } from "./comparators"; diff --git a/scm-ui/ui-components/src/repos/Diff.tsx b/scm-ui/ui-components/src/repos/Diff.tsx index c5cf111f99..e8f4d19060 100644 --- a/scm-ui/ui-components/src/repos/Diff.tsx +++ b/scm-ui/ui-components/src/repos/Diff.tsx @@ -29,7 +29,7 @@ import { getAnchorSelector, getFileNameFromHash } from "./diffs"; import Notification from "../Notification"; import { useTranslation } from "react-i18next"; import { useLocation } from "react-router-dom"; -import useScrollToElement from "../useScrollToElement"; +import { useScrollToElementWithCallback } from "../useScrollToElement"; import { getAnchorId } from "./diff/helpers"; type Props = DiffObjectProps & { @@ -39,6 +39,8 @@ type Props = DiffObjectProps & { fetchNextPage?: () => void; isFetchingNextPage?: boolean; isDataPartial?: boolean; + prevHash?: string; + setPrevHash?: (newHash: string) => void; }; const createKey = (file: FileDiff, ignoreWhitespace?: string) => { @@ -66,6 +68,8 @@ const Diff: FC = ({ fetchNextPage, isFetchingNextPage, isDataPartial, + prevHash, + setPrevHash, ...fileProps }) => { const [t] = useTranslation("repos"); @@ -78,13 +82,17 @@ const Diff: FC = ({ } }, [isFetchingNextPage]); - useScrollToElement( + useScrollToElementWithCallback( contentRef, () => { if (isFetchingNextPage === undefined || isDataPartial === undefined || fetchNextPage === undefined) { return selectFromHash(hash); } + if (prevHash === hash) { + return; + } + const encodedFileName = getFileNameFromHash(hash); if (!encodedFileName) { return; @@ -99,7 +107,12 @@ const Diff: FC = ({ fetchNextPage(); } }, - [hash] + [hash, isDataPartial, isFetchingNextPage, fetchNextPage, diff, prevHash, setPrevHash], + () => { + if (setPrevHash) { + setPrevHash(hash); + } + } ); return ( diff --git a/scm-ui/ui-components/src/repos/LoadingDiff.tsx b/scm-ui/ui-components/src/repos/LoadingDiff.tsx index 630c07fc25..5f889c5cf8 100644 --- a/scm-ui/ui-components/src/repos/LoadingDiff.tsx +++ b/scm-ui/ui-components/src/repos/LoadingDiff.tsx @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ -import React, { FC, useCallback, useState } from "react"; +import React, { FC, useState } from "react"; import { useTranslation } from "react-i18next"; import { NotFoundError, useDiff } from "@scm-manager/ui-api"; import ErrorNotification from "../ErrorNotification"; @@ -33,7 +33,6 @@ import { DiffObjectProps } from "./DiffTypes"; import DiffStatistics from "./DiffStatistics"; import { DiffDropDown } from "../index"; import DiffFileTree from "./diff/DiffFileTree"; -import { FileTree } from "@scm-manager/ui-types"; import { DiffContent, FileTreeContent } from "./diff/styledElements"; import { useHistory, useLocation } from "react-router-dom"; import { getFileNameFromHash } from "./diffs"; @@ -65,14 +64,10 @@ const PartialNotification: FC = ({ fetchNextPage, isFetchingN const LoadingDiff: FC = ({ url, limit, refetchOnWindowFocus, ...props }) => { const [ignoreWhitespace, setIgnoreWhitespace] = useState(false); const [collapsed, setCollapsed] = useState(false); + const [prevHash, setPrevHash] = useState(""); const location = useLocation(); const history = useHistory(); - const fetchNextPageAndResetAnchor = () => { - history.push("#"); - fetchNextPage(); - }; - const evaluateWhiteSpace = () => { return ignoreWhitespace ? "ALL" : "NONE"; }; @@ -83,26 +78,6 @@ const LoadingDiff: FC = ({ url, limit, refetchOnWindowFocus, ...props }) }); const [t] = useTranslation("repos"); - const getFirstFile = useCallback((tree: FileTree): string => { - if (Object.keys(tree.children).length === 0) { - return tree.nodeName; - } - - for (const key in tree.children) { - let path; - if (tree.nodeName !== "") { - path = tree.nodeName + "/"; - } else { - path = tree.nodeName; - } - const result = path + getFirstFile(tree.children[key]); - if (result) { - return result; - } - } - return ""; - }, []); - const ignoreWhitespaces = () => { setIgnoreWhitespace(!ignoreWhitespace); }; @@ -112,6 +87,7 @@ const LoadingDiff: FC = ({ url, limit, refetchOnWindowFocus, ...props }) }; const setFilePath = (path: string) => { + setPrevHash(""); history.push(`#diff-${encodeURIComponent(path)}`); }; @@ -148,10 +124,12 @@ const LoadingDiff: FC = ({ url, limit, refetchOnWindowFocus, ...props }) fetchNextPage={fetchNextPage} isFetchingNextPage={isFetchingNextPage} isDataPartial={data.partial} + prevHash={prevHash} + setPrevHash={setPrevHash} {...props} /> {data.partial ? ( - + ) : null} diff --git a/scm-ui/ui-components/src/useScrollToElement.ts b/scm-ui/ui-components/src/useScrollToElement.ts index c68c632835..c0bdb6df68 100644 --- a/scm-ui/ui-components/src/useScrollToElement.ts +++ b/scm-ui/ui-components/src/useScrollToElement.ts @@ -24,10 +24,12 @@ import { useEffect } from "react"; -const useScrollToElement = ( +// In some cases it is necessary to execute some logic, after successfully jumping to an element +export const useScrollToElementWithCallback = ( contentRef: HTMLElement | null | undefined, elementSelectorResolver: () => string | undefined, - ...dependencies: any + dependencies: unknown[], + callback?: () => void ) => { useEffect(() => { const elementSelector = elementSelectorResolver(); @@ -46,6 +48,10 @@ const useScrollToElement = ( clearInterval(intervalId); const y = element.getBoundingClientRect().top + window.pageYOffset - margin; window.scrollTo({ top: y }); + + if (callback) { + callback(); + } } } }, 200); @@ -57,6 +63,14 @@ const useScrollToElement = ( }, [contentRef, ...dependencies]); }; +const useScrollToElement = ( + contentRef: HTMLElement | null | undefined, + elementSelectorResolver: () => string | undefined, + ...dependencies: any +) => { + useScrollToElementWithCallback(contentRef, elementSelectorResolver, [...dependencies]); +}; + function escapeIdStartingWithNumber(selector: string) { if (selector.startsWith("#")) { return `#${CSS.escape(selector.substring(1))}`;