diff --git a/web/package.json b/web/package.json index 53c3e7410..9f73c13a6 100644 --- a/web/package.json +++ b/web/package.json @@ -47,7 +47,8 @@ "@harness/use-modal": "1.3.0", "@popperjs/core": "^2.4.2", "@projectstorm/react-diagrams-core": "^6.6.0", - "@uiw/react-markdown-preview": "^4.1.2", + "@uiw/react-markdown-editor": "^5.10.1", + "@uiw/react-markdown-preview": "4.1.6", "@urql/exchange-request-policy": "^0.1.3", "anser": "^2.0.1", "classnames": "^2.2.6", diff --git a/web/src/components/DiffViewer/DiffViewer.module.scss b/web/src/components/DiffViewer/DiffViewer.module.scss index b1c92a709..b1d28aac8 100644 --- a/web/src/components/DiffViewer/DiffViewer.module.scss +++ b/web/src/components/DiffViewer/DiffViewer.module.scss @@ -11,6 +11,64 @@ .d2h-file-wrapper { border: 0; + + .d2h-diff-tbody { + &, + tr { + position: relative; + } + } + + &.side-by-side-file-diff { + .d2h-code-side-linenumber.d2h-info { + pointer-events: none; + } + } + + [data-annotated='true'] [data-content-for-line-number], + [data-content-for-line-number=''] { + &, + :hover { + [data-annotation-for-line] { + pointer-events: none; + display: none; + } + } + } + + [data-content-for-line-number] { + cursor: default; + + [data-annotation-for-line] { + position: absolute; + top: 1px; + left: 60px; + display: flex; + height: 14px; + width: 14px; + font-weight: 600; + background: var(--purple-500); + color: var(--white); + text-align: center; + border-radius: 5px; + align-items: center; + justify-content: center; + cursor: pointer; + visibility: hidden; + } + + &:hover [data-annotation-for-line] { + visibility: visible; + transform: scale(1.5); + transition: transform 0.75s; + } + } + + &.line-by-line-file-diff { + [data-annotation-for-line] { + left: 102px; + } + } } .d2h-file-header { @@ -31,10 +89,6 @@ .d2h-code-side-linenumber { width: 56px; } - - .d2h-diff-tbody { - position: relative; - } } &.collapsed { @@ -98,9 +152,24 @@ border-bottom-right-radius: 5px; max-width: calc(100vw - 320px); - - // &[data-display='none'] { - // visibility: hidden; - // } + } +} + +.annotationCell { + box-sizing: border-box; + padding: var(--spacing-medium); + + .annotationCellContainer { + border: 1px solid var(--border-color); + border-radius: 5px; + height: 100%; + padding: var(--spacing-medium); + max-width: 800px; + + &[data-view-style='side-by-side'] { + width: 600px; + position: sticky; + left: var(--spacing-medium); + } } } diff --git a/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts b/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts index 3f4a473d2..b931a322e 100644 --- a/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts +++ b/web/src/components/DiffViewer/DiffViewer.module.scss.d.ts @@ -8,5 +8,7 @@ declare const styles: { readonly offscreen: string readonly fname: string readonly viewLabel: string + readonly annotationCell: string + readonly annotationCellContainer: string } export default styles diff --git a/web/src/components/DiffViewer/DiffViewer.tsx b/web/src/components/DiffViewer/DiffViewer.tsx index 004820a7c..bec4a8415 100644 --- a/web/src/components/DiffViewer/DiffViewer.tsx +++ b/web/src/components/DiffViewer/DiffViewer.tsx @@ -12,13 +12,14 @@ import { CodeIcon } from 'utils/GitUtils' import { PipeSeparator } from 'components/PipeSeparator/PipeSeparator' import css from './DiffViewer.module.scss' -export enum DiffViewStyle { +export enum ViewStyle { SPLIT = 'side-by-side', UNIFIED = 'line-by-line' } const DIFF_HEADER_HEIGHT = 36 const LINE_NUMBER_CLASS = 'diff-viewer-line-number' +const INITIAL_COMMENT_HEIGHT = 200 export const DIFF2HTML_CONFIG = { outputFormat: 'side-by-side', @@ -31,11 +32,12 @@ export const DIFF2HTML_CONFIG = { renderNothingWhenEmpty: false, compiledTemplates: { 'generic-line': HoganJsUtils.compile(` - - + + {{{lineNumber}}} - + +
+
{{#prefix}} {{{prefix}}} @@ -53,17 +55,70 @@ export const DIFF2HTML_CONFIG = { `), + 'side-by-side-file-diff': HoganJsUtils.compile(` +
+
+ {{{filePath}}} +
+
+
+
+ + + {{{diffs.left}}} + +
+
+
+
+
+ + + {{{diffs.right}}} + +
+
+
+
+
+ `), + 'line-by-line-file-diff': HoganJsUtils.compile(` +
+
+ {{{filePath}}} +
+
+
+ + + {{{diffs}}} + +
+
+
+
+ `), 'line-by-line-numbers': HoganJsUtils.compile(` -
{{oldNumber}}
-
{{newNumber}}
+
{{oldNumber}}
+
{{newNumber}}
`) } } as Readonly +interface ContentAnnotationInfo { + left: boolean + right: boolean + lineNumber: number + annotatedElement: HTMLTableRowElement | null + width: number + height: number + nthChild: number +} + interface DiffViewerProps { index: number diff: DiffFile - viewStyle: DiffViewStyle + viewStyle: ViewStyle stickyTopPosition?: number } @@ -85,6 +140,7 @@ export const DiffViewer: React.FC = ({ index, diff, viewStyle, const [diffRenderer, setDiffRenderer] = useState() const { ref: inViewRef, inView } = useInView({ rootMargin: '100px 0px' }) const containerRef = useRef(null) + const [annotations, setAnnotations] = useState([]) const setContainerRef = useCallback( node => { containerRef.current = node @@ -171,29 +227,99 @@ export const DiffViewer: React.FC = ({ index, diff, viewStyle, } else { containerClassList.remove(css.collapsed) - if (parseInt(containerStyle.height) != height) { - containerStyle.height = `${height}px` + const annotationHeights = annotations.reduce((total, annotation) => total + annotation.height, 0) || 0 + const newHeight = Number(height) + annotationHeights + + if (parseInt(containerStyle.height) != newHeight) { + containerStyle.height = `${newHeight}px` } } }, - [collapsed, height, stickyTopPosition] + [collapsed, height, stickyTopPosition, annotations] ) - useEffect(function () { - const onClick = (event: MouseEvent) => { - const target = event.target as HTMLDivElement + useEffect( + function clickToAnnotate() { + const onClick = (event: MouseEvent) => { + const annotationInfo: ContentAnnotationInfo = { + left: false, + right: false, + lineNumber: 0, + annotatedElement: null, + height: 0, + width: 0, + nthChild: 1 + } + const target = event.target as HTMLDivElement + const annotationButton = target?.closest('[data-annotation-for-line]') as HTMLDivElement + const parentRow = annotationButton?.closest('tr') as HTMLTableRowElement + const isSplitView = viewStyle === ViewStyle.SPLIT - if (target.classList.contains(LINE_NUMBER_CLASS)) { - console.log('line number clicked') + if (annotationButton && parentRow) { + if (isSplitView) { + const leftParent = annotationButton.closest('.d2h-file-side-diff.left') + annotationInfo.left = !!leftParent + annotationInfo.right = !leftParent + annotationInfo.lineNumber = Number(annotationButton.dataset.lineNumber) + } else { + const lineInfoTD = annotationButton.closest('td')?.previousElementSibling + const lineNum1 = lineInfoTD?.querySelector('.line-num1') + const lineNum2 = lineInfoTD?.querySelector('.line-num2') + + annotationInfo.left = !!lineNum1?.textContent + annotationInfo.right = !annotationInfo.left + annotationInfo.lineNumber = Number(lineNum1?.textContent || lineNum2?.textContent) + } + + const _height = INITIAL_COMMENT_HEIGHT + + parentRow.dataset.annotated = 'true' // TODO: set to lookup value instead of true + annotationInfo.annotatedElement = parentRow + annotationInfo.height = _height + + const tr = document.createElement('tr') + tr.dataset.isAnnotation = 'true' // TODO: set to lookup value instead of true + + tr.innerHTML = ` + +
Enter your comment or review here...
+ + ` + parentRow.after(tr) + + let node = parentRow as Element + while (node.previousElementSibling) { + annotationInfo.nthChild++ + node = node.previousElementSibling + } + + // Add space in opposit pane (split view only) + if (isSplitView) { + const filesDiff = parentRow.closest('.d2h-files-diff') as HTMLElement + const sideDiff = filesDiff?.querySelector(`div.${annotationInfo.left ? 'right' : 'left'}`) as HTMLElement + const sideRow = sideDiff?.querySelector(`tr:nth-child(${annotationInfo.nthChild})`) + + const tr2 = document.createElement('tr') + tr2.innerHTML = `` + + sideRow?.after(tr2) + } + + console.log(annotationInfo) + + setAnnotations([...annotations, annotationInfo]) + } } - console.log({ event, target }) - } - const containerDOM = containerRef.current as HTMLDivElement - containerDOM.addEventListener('click', onClick) - return () => { - containerDOM.removeEventListener('click', onClick) - } - }, []) + + const containerDOM = containerRef.current as HTMLDivElement + containerDOM.addEventListener('click', onClick) + + return () => { + containerDOM.removeEventListener('click', onClick) + } + }, + [viewStyle, annotations] + ) return ( {getString('loading')}}> - + ) diff --git a/web/src/framework/strings/stringTypes.ts b/web/src/framework/strings/stringTypes.ts index 77045317c..b14af3e69 100644 --- a/web/src/framework/strings/stringTypes.ts +++ b/web/src/framework/strings/stringTypes.ts @@ -84,6 +84,7 @@ export interface StringsMap { failedToDeleteBranch: string fileDeleted: string files: string + filesChanged: string findATag: string findBranch: string findOrCreateBranch: string diff --git a/web/src/hooks/useEventListener.ts b/web/src/hooks/useEventListener.ts new file mode 100644 index 000000000..7a96abdda --- /dev/null +++ b/web/src/hooks/useEventListener.ts @@ -0,0 +1,15 @@ +import { useEffect } from 'react' + +export function useEventListener( + type: K, + listener: (this: HTMLElement, ev: HTMLElementEventMap[K]) => Unknown, + element: HTMLElement = window as unknown as HTMLElement, + options?: boolean | AddEventListenerOptions +) { + useEffect(() => { + element.addEventListener(type, listener, options) + return () => { + element.removeEventListener(type, listener) + } + }, [element, type, listener, options]) +} diff --git a/web/src/i18n/strings.en.yaml b/web/src/i18n/strings.en.yaml index 63e5ec4e3..25c67b081 100644 --- a/web/src/i18n/strings.en.yaml +++ b/web/src/i18n/strings.en.yaml @@ -183,3 +183,4 @@ rejected: Rejected yours: Yours all: All scrollToTop: Scroll to top +filesChanged: File Changed diff --git a/web/src/pages/PullRequest/PullRequest.tsx b/web/src/pages/PullRequest/PullRequest.tsx index 45cb13e6c..4e6876e67 100644 --- a/web/src/pages/PullRequest/PullRequest.tsx +++ b/web/src/pages/PullRequest/PullRequest.tsx @@ -18,7 +18,7 @@ import css from './PullRequest.module.scss' enum PullRequestSection { CONVERSATION = 'conversation', COMMITS = 'commits', - DIFFS = 'diffs' + FILES_CHANGED = 'files' } export default function PullRequest() { @@ -95,8 +95,8 @@ export default function PullRequest() { panel: }, { - id: PullRequestSection.DIFFS, - title: , + id: PullRequestSection.FILES_CHANGED, + title: , panel: } ]} diff --git a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss index dcf1db5a9..fc30fa2a1 100644 --- a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss +++ b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss @@ -35,9 +35,8 @@ } .filesMenu { - max-height: 350px; + max-height: 400px; overflow: auto; - padding: var(--spacing-xsmall) !important; :global { .bp3-menu-item:hover { @@ -50,3 +49,7 @@ align-items: center; } } + +.popover { + border-radius: 8px !important; +} diff --git a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss.d.ts b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss.d.ts index 254455bff..fed594835 100644 --- a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss.d.ts +++ b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.module.scss.d.ts @@ -8,5 +8,6 @@ declare const styles: { readonly layout: string readonly filesMenu: string readonly menuItem: string + readonly popover: string } export default styles diff --git a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.tsx b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.tsx index 8ac0433a0..b71b37e8f 100644 --- a/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.tsx +++ b/web/src/pages/PullRequest/PullRequestDiff/PullRequestDiff.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useMemo, useState } from 'react' +import React, { useCallback, useEffect, useMemo, useState } from 'react' import { Container, FlexExpander, @@ -23,55 +23,41 @@ import type { DiffFile } from 'diff2html/lib/types' import { useStrings } from 'framework/strings' import { CodeIcon, GitInfoProps } from 'utils/GitUtils' import { ButtonRoleProps, formatNumber, waitUntil } from 'utils/Utils' -import { DiffViewer, DIFF2HTML_CONFIG, DiffViewStyle } from 'components/DiffViewer/DiffViewer' +import { DiffViewer, DIFF2HTML_CONFIG, ViewStyle } from 'components/DiffViewer/DiffViewer' +import { useEventListener } from 'hooks/useEventListener' import { UserPreference, useUserPreference } from 'hooks/useUserPreference' import { PipeSeparator } from 'components/PipeSeparator/PipeSeparator' import { PullRequestTabContentWrapper } from '../PullRequestTabContentWrapper' -import diffExample from 'raw-loader!./example2.diff' +import diffExample from 'raw-loader!./example.diff' import css from './PullRequestDiff.module.scss' const STICKY_TOP_POSITION = 64 +const STICKY_HEADER_HEIGHT = 150 export const PullRequestDiff: React.FC> = () => { const { getString } = useStrings() - const [viewStyle, setViewStyle] = useUserPreference(UserPreference.DIFF_VIEW_STYLE, DiffViewStyle.SPLIT) + const [viewStyle, setViewStyle] = useUserPreference(UserPreference.DIFF_VIEW_STYLE, ViewStyle.SPLIT) const [diffs, setDiffs] = useState([]) - const [stickyInAction, setStickyInAction] = useState(false) - const diffStats = useMemo(() => { - return (diffs || []).reduce( - (obj, diff) => { - obj.addedLines += diff.addedLines - obj.deletedLines += diff.deletedLines - return obj - }, - { addedLines: 0, deletedLines: 0 } - ) - }, [diffs]) + const [isSticky, setSticky] = useState(false) + const diffStats = useMemo( + () => + (diffs || []).reduce( + (obj, diff) => { + obj.addedLines += diff.addedLines + obj.deletedLines += diff.deletedLines + return obj + }, + { addedLines: 0, deletedLines: 0 } + ), + [diffs] + ) - useEffect(() => { - setDiffs(Diff2Html.parse(diffExample, DIFF2HTML_CONFIG)) - }, []) + useEffect(() => setDiffs(Diff2Html.parse(diffExample, DIFF2HTML_CONFIG)), []) - useEffect(() => { - const onScroll = () => { - if (window.scrollY >= 150) { - if (!stickyInAction) { - setStickyInAction(true) - } - } else { - if (stickyInAction) { - setStickyInAction(false) - } - } - } - window.addEventListener('scroll', onScroll) - - return () => { - window.removeEventListener('scroll', onScroll) - } - }, [stickyInAction]) - - // console.log({ diffs, viewStyle }) + useEventListener( + 'scroll', + useCallback(() => setSticky(window.scrollY >= STICKY_HEADER_HEIGHT), []) + ) return ( @@ -87,59 +73,69 @@ export const PullRequestDiff: React.FC - {diffs?.map((diff, index) => ( - } - labelElement={ - - {!!diff.addedLines && ( - - +{diff.addedLines} - - )} - {!!diff.addedLines && !!diff.deletedLines && } - {!!diff.deletedLines && ( - - -{diff.deletedLines} - - )} - - } - text={ - diff.isDeleted - ? diff.oldName - : diff.isRename - ? `${diff.oldName} -> ${diff.newName}` - : diff.newName - } - onClick={() => { - const containerDOM = document.getElementById(`file-diff-container-${index}`) + + + {diffs?.map((diff, index) => ( + } + labelElement={ + + {!!diff.addedLines && ( + + +{diff.addedLines} + + )} + {!!diff.addedLines && !!diff.deletedLines && } + {!!diff.deletedLines && ( + + -{diff.deletedLines} + + )} + + } + text={ + diff.isDeleted + ? diff.oldName + : diff.isRename + ? `${diff.oldName} -> ${diff.newName}` + : diff.newName + } + onClick={() => { + // When an item is clicked, do these: + // 1. Scroll into the diff container of the file. + // The diff content might not be rendered yet since it's off-screen + // 2. Wait until its content is rendered + // 3. Adjust scroll position as when diff content is rendered, current + // window scroll position might push diff content up, we need to push + // it down again to make sure first line of content is visible and not + // covered by sticky headers + const containerDOM = document.getElementById(`file-diff-container-${index}`) - if (containerDOM) { - containerDOM.scrollIntoView() - waitUntil( - () => !!containerDOM.querySelector('[data-rendered="true"]'), - () => { - containerDOM.scrollIntoView() - // Fix scrolling position messes up with sticky header - const { y } = containerDOM.getBoundingClientRect() - if (y - STICKY_TOP_POSITION < 1) { - if (STICKY_TOP_POSITION) { - window.scroll({ top: window.scrollY - STICKY_TOP_POSITION }) + if (containerDOM) { + containerDOM.scrollIntoView() + + waitUntil( + () => !!containerDOM.querySelector('[data-rendered="true"]'), + () => { + containerDOM.scrollIntoView() + + if (containerDOM.getBoundingClientRect().y - STICKY_TOP_POSITION < 1) { + if (STICKY_TOP_POSITION) { + window.scroll({ top: window.scrollY - STICKY_TOP_POSITION }) + } } } - } - ) - } - }} - /> - ))} - + ) + } + }} + /> + ))} + + } - tooltipProps={{ interactionKind: 'click', hasBackdrop: true }}> + tooltipProps={{ interactionKind: 'click', hasBackdrop: true, popoverClassName: css.popover }}> ), @@ -159,10 +155,10 @@ export const PullRequestDiff: React.FC { - setViewStyle(DiffViewStyle.SPLIT) + setViewStyle(ViewStyle.SPLIT) window.scroll({ top: 0 }) }}> {getString('pr.split')} @@ -170,10 +166,10 @@ export const PullRequestDiff: React.FC { - setViewStyle(DiffViewStyle.UNIFIED) + setViewStyle(ViewStyle.UNIFIED) window.scroll({ top: 0 }) }}> {getString('pr.unified')} @@ -182,6 +178,7 @@ export const PullRequestDiff: React.FC } + tooltipProps={{ interactionKind: 'click' }} iconProps={{ size: 14, padding: { right: 3 } }} rightIconProps={{ size: 13, padding: { left: 0 } }} padding={{ left: 'small' }} @@ -191,7 +188,7 @@ export const PullRequestDiff: React.FC - {stickyInAction && ( + {isSticky && (