From 94d62ab2f0a8aa66770ed6373481b4bf2574e6e9 Mon Sep 17 00:00:00 2001 From: Philippe Perrin Date: Wed, 10 Aug 2022 19:06:16 +0200 Subject: [PATCH] SONAR-17169 Improve scrolling architecture in issues page --- .../IssueSourceViewerScrollContext.tsx | 30 +++++ .../js/apps/issues/components/IssuesApp.tsx | 28 +--- .../issues/components/IssuesSourceViewer.tsx | 124 +++++++++++++----- .../IssuesSourceViewer-test.tsx.snap | 44 +++++-- .../ComponentSourceSnippetGroupViewer.tsx | 53 ++++---- .../CrossComponentSourceViewer.tsx | 3 - ...ComponentSourceSnippetGroupViewer-test.tsx | 9 +- .../CrossComponentSourceViewer-test.tsx | 1 - .../src/main/js/apps/issues/utils.ts | 1 + .../SourceViewer/components/LineCode.tsx | 37 ++---- .../__snapshots__/LineCode-test.tsx.snap | 6 +- .../js/components/issue/IssueMessageBox.tsx | 72 ++++------ .../js/components/rules/RuleTabViewer.tsx | 2 +- 13 files changed, 233 insertions(+), 177 deletions(-) create mode 100644 server/sonar-web/src/main/js/apps/issues/components/IssueSourceViewerScrollContext.tsx diff --git a/server/sonar-web/src/main/js/apps/issues/components/IssueSourceViewerScrollContext.tsx b/server/sonar-web/src/main/js/apps/issues/components/IssueSourceViewerScrollContext.tsx new file mode 100644 index 00000000000..9a4a1a23619 --- /dev/null +++ b/server/sonar-web/src/main/js/apps/issues/components/IssueSourceViewerScrollContext.tsx @@ -0,0 +1,30 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +import React from 'react'; + +export interface IssueSourceViewerScrollContextInterface { + registerPrimaryLocationRef: React.Ref; + registerSelectedSecondaryLocationRef: React.Ref; +} + +export const IssueSourceViewerScrollContext = React.createContext< + IssueSourceViewerScrollContextInterface | undefined +>(undefined); diff --git a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx index 46fea9e8fb1..e1b7e1caaa5 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx +++ b/server/sonar-web/src/main/js/apps/issues/components/IssuesApp.tsx @@ -89,7 +89,6 @@ import { parseQuery, Query, saveMyIssues, - scrollToIssue, serializeQuery, shouldOpenSonarSourceSecurityFacet, shouldOpenStandardsChildFacet, @@ -227,13 +226,6 @@ export class App extends React.PureComponent { ) { this.fetchFirstIssues(); this.setState({ checkAll: false }); - } else if ( - !this.state.openIssue && - (prevState.selected !== this.state.selected || prevState.openIssue) - ) { - // if user simply selected another issue - // or if user went from the source code back to the list of issues - this.scrollToSelectedIssue(); } else if (openIssue && openIssue.key !== this.state.selected) { this.setState({ locationsNavigator: false, @@ -391,14 +383,11 @@ export class App extends React.PureComponent { }; if (this.state.openIssue) { if (path.query.open && path.query.open === this.state.openIssue.key) { - this.setState( - { - locationsNavigator: false, - selectedFlowIndex: undefined, - selectedLocationIndex: undefined - }, - this.scrollToSelectedIssue - ); + this.setState({ + locationsNavigator: false, + selectedFlowIndex: undefined, + selectedLocationIndex: undefined + }); } else { this.props.router.replace(path); } @@ -429,13 +418,6 @@ export class App extends React.PureComponent { } }; - scrollToSelectedIssue = (smooth = true) => { - const { selected } = this.state; - if (selected) { - scrollToIssue(selected, smooth); - } - }; - createdAfterIncludesTime = () => Boolean(this.props.location.query.createdAfter?.includes('T')); fetchIssuesHelper = (query: RawQuery) => { diff --git a/server/sonar-web/src/main/js/apps/issues/components/IssuesSourceViewer.tsx b/server/sonar-web/src/main/js/apps/issues/components/IssuesSourceViewer.tsx index ee79ab27059..8e0999fd403 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/IssuesSourceViewer.tsx +++ b/server/sonar-web/src/main/js/apps/issues/components/IssuesSourceViewer.tsx @@ -22,6 +22,7 @@ import { BranchLike } from '../../../types/branch-like'; import { Issue } from '../../../types/types'; import CrossComponentSourceViewer from '../crossComponentSourceViewer/CrossComponentSourceViewer'; import { getLocations, getSelectedLocation } from '../utils'; +import { IssueSourceViewerScrollContext } from './IssueSourceViewerScrollContext'; export interface IssuesSourceViewerProps { branchLike: BranchLike | undefined; @@ -34,39 +35,96 @@ export interface IssuesSourceViewerProps { selectedLocationIndex: number | undefined; } -export default function IssuesSourceViewer(props: IssuesSourceViewerProps) { - const { - openIssue, - selectedFlowIndex, - selectedLocationIndex, - locationsNavigator, - branchLike, - issues - } = props; +export default class IssuesSourceViewer extends React.PureComponent { + primaryLocationRef?: HTMLElement; + selectedSecondaryLocationRef?: HTMLElement; - const locations = getLocations(openIssue, selectedFlowIndex).map((loc, index) => { - loc.index = index; - return loc; - }); - const selectedLocation = getSelectedLocation(openIssue, selectedFlowIndex, selectedLocationIndex); + componentDidUpdate() { + if (this.props.selectedLocationIndex === -1) { + this.refreshScroll(); + } + } - const highlightedLocationMessage = - locationsNavigator && selectedLocationIndex !== undefined - ? selectedLocation && { index: selectedLocationIndex, text: selectedLocation.msg } - : undefined; - return ( -
- -
- ); + registerPrimaryLocationRef = (ref: HTMLElement) => { + this.primaryLocationRef = ref; + + if (ref) { + this.refreshScroll(); + } + }; + + registerSelectedSecondaryLocationRef = (ref: HTMLElement) => { + this.selectedSecondaryLocationRef = ref; + + if (ref) { + this.refreshScroll(); + } + }; + + refreshScroll() { + const { selectedLocationIndex } = this.props; + + if ( + selectedLocationIndex !== undefined && + selectedLocationIndex !== -1 && + this.selectedSecondaryLocationRef + ) { + this.selectedSecondaryLocationRef.scrollIntoView({ + behavior: 'smooth', + block: 'center', + inline: 'nearest' + }); + } else if (this.primaryLocationRef) { + this.primaryLocationRef.scrollIntoView({ + block: 'center', + inline: 'nearest' + }); + } + } + + render() { + const { + openIssue, + selectedFlowIndex, + selectedLocationIndex, + locationsNavigator, + branchLike, + issues + } = this.props; + + const locations = getLocations(openIssue, selectedFlowIndex).map((loc, index) => { + loc.index = index; + return loc; + }); + + const selectedLocation = getSelectedLocation( + openIssue, + selectedFlowIndex, + selectedLocationIndex + ); + + const highlightedLocationMessage = + locationsNavigator && selectedLocationIndex !== undefined + ? selectedLocation && { index: selectedLocationIndex, text: selectedLocation.msg } + : undefined; + + return ( + + + + ); + } } diff --git a/server/sonar-web/src/main/js/apps/issues/components/__tests__/__snapshots__/IssuesSourceViewer-test.tsx.snap b/server/sonar-web/src/main/js/apps/issues/components/__tests__/__snapshots__/IssuesSourceViewer-test.tsx.snap index 17f3c6d7887..f835fb8a7b3 100644 --- a/server/sonar-web/src/main/js/apps/issues/components/__tests__/__snapshots__/IssuesSourceViewer-test.tsx.snap +++ b/server/sonar-web/src/main/js/apps/issues/components/__tests__/__snapshots__/IssuesSourceViewer-test.tsx.snap @@ -1,7 +1,14 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`should render CrossComponentSourceViewer correctly 1`] = ` -
+ -
+ `; exports[`should render SourceViewer correctly: all secondary locations on same line 1`] = ` -
+ -
+ `; exports[`should render SourceViewer correctly: default 1`] = ` -
+ -
+ `; exports[`should render SourceViewer correctly: single secondary location 1`] = ` -
+ -
+ `; diff --git a/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/ComponentSourceSnippetGroupViewer.tsx b/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/ComponentSourceSnippetGroupViewer.tsx index 892849613c1..a8553151e0f 100644 --- a/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/ComponentSourceSnippetGroupViewer.tsx +++ b/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/ComponentSourceSnippetGroupViewer.tsx @@ -37,6 +37,7 @@ import { SourceLine, SourceViewerFile } from '../../../types/types'; +import { IssueSourceViewerScrollContext } from '../components/IssueSourceViewerScrollContext'; import IssueSourceViewerHeader from './IssueSourceViewerHeader'; import SnippetViewer from './SnippetViewer'; import { @@ -67,7 +68,6 @@ interface Props { line: number ) => React.ReactNode; snippetGroup: SnippetGroup; - selectedLocationIndex: number | undefined; } interface State { @@ -206,13 +206,7 @@ export default class ComponentSourceSnippetGroupViewer extends React.PureCompone }; renderIssuesList = (line: SourceLine) => { - const { - isLastOccurenceOfPrimaryComponent, - issue, - issuesByLine, - snippetGroup, - selectedLocationIndex - } = this.props; + const { isLastOccurenceOfPrimaryComponent, issue, issuesByLine, snippetGroup } = this.props; const locations = issue.component === snippetGroup.component.key && issue.textRange !== undefined ? locationsByLine([issue]) @@ -226,15 +220,21 @@ export default class ComponentSourceSnippetGroupViewer extends React.PureCompone return ( issuesForLine.length > 0 && (
- {issuesForLine.map(issueToDisplay => ( - 0)} - key={issueToDisplay.key} - issue={issueToDisplay} - onClick={this.props.onIssueSelect} - selectedLocationIndex={selectedLocationIndex} - /> - ))} + {issuesForLine.map(issueToDisplay => { + const isSelectedIssue = issueToDisplay.key === issue.key; + return ( + + {ctx => ( + 0)} + issue={issueToDisplay} + onClick={this.props.onIssueSelect} + ref={isSelectedIssue ? ctx?.registerPrimaryLocationRef : undefined} + /> + )} + + ); + })}
) ); @@ -246,8 +246,7 @@ export default class ComponentSourceSnippetGroupViewer extends React.PureCompone isLastOccurenceOfPrimaryComponent, issue, lastSnippetGroup, - snippetGroup, - selectedLocationIndex + snippetGroup } = this.props; const { additionalLines, loading, snippets } = this.state; const locations = @@ -280,12 +279,16 @@ export default class ComponentSourceSnippetGroupViewer extends React.PureCompone /> {issue.component === snippetGroup.component.key && issue.textRange === undefined && ( - + + {ctx => ( + + )} + )} {snippetLines.map((snippet, index) => ( void; onLocationSelect: (index: number) => void; selectedFlowIndex: number | undefined; - selectedLocationIndex: number | undefined; } interface State { @@ -184,7 +183,6 @@ export default class CrossComponentSourceViewer extends React.PureComponent ); diff --git a/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/__tests__/ComponentSourceSnippetGroupViewer-test.tsx b/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/__tests__/ComponentSourceSnippetGroupViewer-test.tsx index 57e292a25d2..260173f7191 100644 --- a/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/__tests__/ComponentSourceSnippetGroupViewer-test.tsx +++ b/server/sonar-web/src/main/js/apps/issues/crossComponentSourceViewer/__tests__/ComponentSourceSnippetGroupViewer-test.tsx @@ -148,7 +148,13 @@ it('should render file-level issue correctly', () => { } }); - expect(wrapper.find(IssueMessageBox).exists()).toBe(true); + expect( + wrapper + .find('ContextConsumer') + .dive() + .find(IssueMessageBox) + .exists() + ).toBe(true); }); it('should expand block', async () => { @@ -294,7 +300,6 @@ function shallowRender(props: Partial = {}) !location.msg); } +// TODO: drop as it's useless now export function scrollToIssue(issue: string, smooth = true) { const element = document.querySelector(`[data-issue="${issue}"]`); if (element) { diff --git a/server/sonar-web/src/main/js/components/SourceViewer/components/LineCode.tsx b/server/sonar-web/src/main/js/components/SourceViewer/components/LineCode.tsx index 498b2df1e9a..713c0ea288b 100644 --- a/server/sonar-web/src/main/js/components/SourceViewer/components/LineCode.tsx +++ b/server/sonar-web/src/main/js/components/SourceViewer/components/LineCode.tsx @@ -19,6 +19,7 @@ */ import classNames from 'classnames'; import * as React from 'react'; +import { IssueSourceViewerScrollContext } from '../../../apps/issues/components/IssueSourceViewerScrollContext'; import { LinearIssueLocation, SourceLine } from '../../../types/types'; import LocationIndex from '../../common/LocationIndex'; import Tooltip from '../../controls/Tooltip'; @@ -38,35 +39,8 @@ interface Props { } export default class LineCode extends React.PureComponent> { - activeMarkerNode?: HTMLElement | null; symbols?: NodeListOf; - componentDidMount() { - if (this.activeMarkerNode) { - this.activeMarkerNode.scrollIntoView({ - behavior: 'smooth', - block: 'center', - inline: 'center' - }); - } - } - - componentDidUpdate(prevProps: Props) { - if ( - this.props.highlightedLocationMessage && - (!prevProps.highlightedLocationMessage || - prevProps.highlightedLocationMessage.index !== - this.props.highlightedLocationMessage.index) && - this.activeMarkerNode - ) { - this.activeMarkerNode.scrollIntoView({ - behavior: 'smooth', - block: 'center', - inline: 'center' - }); - } - } - nodeNodeRef = (el: HTMLElement | null) => { if (el) { this.attachEvents(el); @@ -105,7 +79,6 @@ export default class LineCode extends React.PureComponent onLocationSelect(index) : undefined; - const ref = selected ? (node: HTMLElement | null) => (this.activeMarkerNode = node) : undefined; return ( @@ -114,7 +87,13 @@ export default class LineCode extends React.PureComponent - {index + 1} + + {ctx => ( + + {index + 1} + + )} + ); diff --git a/server/sonar-web/src/main/js/components/SourceViewer/components/__tests__/__snapshots__/LineCode-test.tsx.snap b/server/sonar-web/src/main/js/components/SourceViewer/components/__tests__/__snapshots__/LineCode-test.tsx.snap index fa518fddd0e..2dcef7709c8 100644 --- a/server/sonar-web/src/main/js/components/SourceViewer/components/__tests__/__snapshots__/LineCode-test.tsx.snap +++ b/server/sonar-web/src/main/js/components/SourceViewer/components/__tests__/__snapshots__/LineCode-test.tsx.snap @@ -117,9 +117,9 @@ exports[`render code: with secondary location 1`] = ` onClick={[Function]} selected={false} > - - 2 - + + + void; - selectedLocationIndex?: number; } -export default class IssueMessageBox extends React.Component { - messageBoxRef: React.RefObject = React.createRef(); +export function IssueMessageBox(props: IssueMessageBoxProps, ref: React.ForwardedRef) { + const { issue, selected } = props; - componentDidMount() { - if (this.props.selected && this.messageBoxRef.current) { - this.messageBoxRef.current.scrollIntoView({ - block: 'center' - }); - } - } - - componentDidUpdate(prevProps: IssueMessageBoxProps) { - if ( - this.messageBoxRef.current && - ((prevProps.selected !== this.props.selected && this.props.selected) || - (prevProps.selectedLocationIndex !== this.props.selectedLocationIndex && - this.props.selectedLocationIndex === -1)) - ) { - this.messageBoxRef.current.scrollIntoView({ - block: 'center' - }); - } - } - - render() { - const { issue, selected } = this.props; - return ( -
this.props.onClick(issue.key)} - role="region" - ref={this.messageBoxRef} - aria-label={issue.message}> - - {issue.message} -
- ); - } + return ( +
props.onClick(issue.key)} + role="region" + ref={ref} + aria-label={issue.message}> + + {issue.message} +
+ ); } + +export default React.forwardRef(IssueMessageBox); diff --git a/server/sonar-web/src/main/js/components/rules/RuleTabViewer.tsx b/server/sonar-web/src/main/js/components/rules/RuleTabViewer.tsx index f4b9cdf4f1d..4adc421d9e3 100644 --- a/server/sonar-web/src/main/js/components/rules/RuleTabViewer.tsx +++ b/server/sonar-web/src/main/js/components/rules/RuleTabViewer.tsx @@ -119,7 +119,7 @@ export class RuleTabViewer extends React.PureComponent { + computeState = (prevState: State, resetSelectedTab = false) => { const { ruleDetails, currentUser: { isLoggedIn, dismissedNotices } -- 2.39.5