From 3046fd4ddf604769a75c380f5e5060d91fd5d8cd Mon Sep 17 00:00:00 2001 From: Alexis Jacomy Date: Tue, 9 Jul 2024 17:48:29 +0200 Subject: [PATCH] ui-manchette: improves code readability This commit fixes various comments from my review for PR #86. The PR has been merged while I was reviewing it, so we decided I can directly submit a new one with my fixes. Details: - Fixes all typos from proportionnal to proportional - Fixes React.XXX as import { XXX } from 'react' - Refactors Manchette.tsx code to have a single, more readable setState - Removes magic zoom boundaries and replace them with proper consts - Fixes operationalPointsHeight so that it does not mutate things anymore - Rewrites calcOperationalPointsToDisplay with simpler code and some comment, to help make it maintainable - Actually stops displaying OperationalPoint with display: false --- ui-manchette/src/components/Manchette.tsx | 153 +++++++++++------- .../src/components/OperationalPoint.tsx | 11 +- ui-manchette/src/components/consts.ts | 8 + ui-manchette/src/components/helpers.ts | 87 +++++----- ui-manchette/src/types.ts | 5 +- 5 files changed, 157 insertions(+), 107 deletions(-) diff --git a/ui-manchette/src/components/Manchette.tsx b/ui-manchette/src/components/Manchette.tsx index e069e9ec..6aedfb4f 100644 --- a/ui-manchette/src/components/Manchette.tsx +++ b/ui-manchette/src/components/Manchette.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef, useState } from 'react'; +import React, { FC, useCallback, useEffect, useRef, useState } from 'react'; import cx from 'classnames'; import OperationalPointList from './OperationalPointList'; import type { @@ -15,7 +15,13 @@ import { operationalPointsHeight, } from './helpers'; import type { OperationalPoint, SpaceScale } from '@osrd-project/ui-spacetimechart/dist/lib/types'; -import { INITIAL_OP_LIST_HEIGHT, INITIAL_SPACE_TIME_CHART_HEIGHT } from './consts'; +import { + INITIAL_OP_LIST_HEIGHT, + INITIAL_SPACE_TIME_CHART_HEIGHT, + MAX_ZOOM_Y, + MIN_ZOOM_Y, + ZOOM_Y_DELTA, +} from './consts'; import { getDiff } from '../utils/vector'; import { useIsOverflow } from '../hooks/useIsOverFlow'; type ManchetteProps = { @@ -24,57 +30,72 @@ type ManchetteProps = { }; import usePaths from '../hooks/usePaths'; -const Manchette: React.FC = ({ operationalPoints, projectPathTrainResult }) => { +const Manchette: FC = ({ operationalPoints, projectPathTrainResult }) => { const manchette = useRef(null); - const [zoomY, setZoomY] = useState(1); - - const xZoomLevel = 1; - - const [xOffset, setXOffset] = useState(0); - - const [yOffset, setYOffset] = useState(0); - const [panning, setPanning] = useState<{ initialOffset: { x: number; y: number } } | null>(null); - - const [scrollPosition, setScrollPosition] = useState(0); - - const [operationalPointsToDisplay, setOperationalPointsToDisplay] = useState< - StyledOperationalPointType[] - >([]); - - const [operationalPointsChart, setOperationalPointsChart] = useState([]); - const [isProportionnal, setIsProportionnal] = useState(false); - - const [panY, setPanY] = useState(false); - - const [scales, setScales] = useState([]); - - useIsOverflow(manchette, (isOverflowFromCallback) => { - setPanY(isOverflowFromCallback); + const [state, setState] = useState<{ + xZoom: number; + yZoom: number; + xOffset: number; + yOffset: number; + panY: boolean; + panning: { initialOffset: { x: number; y: number } } | null; + scrollPosition: number; + isProportional: boolean; + operationalPointsChart: OperationalPoint[]; + operationalPointsToDisplay: StyledOperationalPointType[]; + scales: SpaceScale[]; + }>({ + xZoom: 1, + yZoom: 1, + xOffset: 0, + yOffset: 0, + panning: null, + scrollPosition: 0, + isProportional: false, + operationalPointsChart: [], + operationalPointsToDisplay: [], + panY: false, + scales: [], }); + const { + xZoom, + yZoom, + xOffset, + yOffset, + panY, + panning, + scrollPosition, + isProportional, + operationalPointsChart, + operationalPointsToDisplay, + scales, + } = state; const paths = usePaths(projectPathTrainResult); - const zoomYIn = () => { - if (zoomY < 10.5) setZoomY(zoomY + 0.5); - }; - - const zoomYOut = () => { - if (zoomY > 1) setZoomY(zoomY - 0.5); - }; - const handleScroll = () => { + const zoomYIn = useCallback(() => { + if (yZoom < MAX_ZOOM_Y) setState((state) => ({ ...state, yZoom: yZoom + ZOOM_Y_DELTA })); + }, [yZoom]); + const zoomYOut = useCallback(() => { + if (yZoom > MIN_ZOOM_Y) setState((state) => ({ ...state, yZoom: yZoom - ZOOM_Y_DELTA })); + }, [yZoom]); + const handleScroll = useCallback(() => { if (manchette.current) { const { scrollTop } = manchette.current; if (scrollTop || scrollTop === 0) { - setScrollPosition(scrollTop); - setYOffset(scrollTop); + setState((state) => ({ ...state, scrollPosition: scrollTop, yOffset: scrollTop })); } } - }; + }, []); + const toggleMode = useCallback(() => { + setState((state) => ({ ...state, isProportional: !state.isProportional })); + }, []); + const checkOverflow = useCallback((isOverflowFromCallback: boolean) => { + setState((state) => ({ ...state, panY: isOverflowFromCallback })); + }, []); - const toggleMode = () => { - setIsProportionnal(!isProportionnal); - }; + useIsOverflow(manchette, checkOverflow); useEffect(() => { window.addEventListener('scroll', handleScroll, { passive: true }); @@ -82,23 +103,31 @@ const Manchette: React.FC = ({ operationalPoints, projectPathTra return () => { window.removeEventListener('scroll', handleScroll); }; - }, []); + }, [handleScroll]); useEffect(() => { const computedOperationalPoints = calcOperationalPointsToDisplay( operationalPoints, - isProportionnal, - zoomY + isProportional, + yZoom ); - setOperationalPointsToDisplay( - operationalPointsHeight(computedOperationalPoints, zoomY, isProportionnal) - ); - setOperationalPointsChart(getOperationalPointsWithPosition(computedOperationalPoints)); - }, [operationalPoints, isProportionnal, zoomY]); + setState((state) => ({ + ...state, + operationalPointsChart: getOperationalPointsWithPosition(computedOperationalPoints), + operationalPointsToDisplay: operationalPointsHeight( + computedOperationalPoints, + yZoom, + isProportional + ), + })); + }, [operationalPoints, isProportional, yZoom]); useEffect(() => { - setScales(getScales(operationalPointsChart, isProportionnal, zoomY)); + setState((state) => ({ + ...state, + scales: getScales(operationalPointsChart, isProportional, yZoom), + })); // eslint-disable-next-line react-hooks/exhaustive-deps }, [operationalPointsChart]); @@ -121,7 +150,7 @@ const Manchette: React.FC = ({ operationalPoints, projectPathTra @@ -129,7 +158,7 @@ const Manchette: React.FC = ({ operationalPoints, projectPathTra
@@ -160,18 +189,20 @@ const Manchette: React.FC = ({ operationalPoints, projectPathTra timeOrigin={Math.min( ...projectPathTrainResult.map((p) => +new Date(p.departure_time)) )} - timeScale={60000 / xZoomLevel} + timeScale={60000 / xZoom} xOffset={xOffset} yOffset={-scrollPosition + 14} onPan={({ initialPosition, position, isPanning }) => { const diff = getDiff(initialPosition, position); + const newState = { ...state }; + if (!isPanning) { - setPanning(null); + newState.panning = null; } else if (!panning) { - setPanning({ initialOffset: { x: xOffset, y: yOffset } }); + newState.panning = { initialOffset: { x: xOffset, y: yOffset } }; } else { const { initialOffset } = panning; - setXOffset(initialOffset.x + diff.x); + newState.xOffset = initialOffset.x + diff.x; if (panY) { const newYPos = initialOffset.y - diff.y; @@ -180,14 +211,16 @@ const Manchette: React.FC = ({ operationalPoints, projectPathTra newYPos >= 0 && newYPos + INITIAL_SPACE_TIME_CHART_HEIGHT <= manchette.current?.scrollHeight ) { - setYOffset(newYPos); - setScrollPosition(yOffset); + newState.yOffset = newYPos; + newState.scrollPosition = newYPos; if (manchette.current && manchette.current.scrollHeight) { manchette.current.scrollTop = newYPos; } } } } + + setState(newState); }} > {paths.map((path, i) => ( diff --git a/ui-manchette/src/components/OperationalPoint.tsx b/ui-manchette/src/components/OperationalPoint.tsx index 9d300653..1920c400 100644 --- a/ui-manchette/src/components/OperationalPoint.tsx +++ b/ui-manchette/src/components/OperationalPoint.tsx @@ -1,9 +1,16 @@ import React from 'react'; -import { OperationalPointType } from '../types'; +import { StyledOperationalPointType } from '../types'; import '@osrd-project/ui-core/dist/theme.css'; import { positionMmToKm } from './helpers'; -const OperationalPoint: React.FC = ({ extensions, id, position }) => { +const OperationalPoint: React.FC = ({ + extensions, + id, + position, + display, +}) => { + if (!display) return null; + return (
{positionMmToKm(position)}
diff --git a/ui-manchette/src/components/consts.ts b/ui-manchette/src/components/consts.ts index 4991f61f..f8e0780e 100644 --- a/ui-manchette/src/components/consts.ts +++ b/ui-manchette/src/components/consts.ts @@ -2,3 +2,11 @@ export const BASE_OP_HEIGHT = 32; export const BASE_KM_HEIGHT = 8; export const INITIAL_OP_LIST_HEIGHT = 521; export const INITIAL_SPACE_TIME_CHART_HEIGHT = INITIAL_OP_LIST_HEIGHT + 40; + +export const MIN_ZOOM_X = 1; +export const MAX_ZOOM_X = 1; +export const ZOOM_X_DELTA = 0.5; + +export const MIN_ZOOM_Y = 1; +export const MAX_ZOOM_Y = 10.5; +export const ZOOM_Y_DELTA = 0.5; diff --git a/ui-manchette/src/components/helpers.ts b/ui-manchette/src/components/helpers.ts index ec982a3a..437e6ac0 100644 --- a/ui-manchette/src/components/helpers.ts +++ b/ui-manchette/src/components/helpers.ts @@ -8,66 +8,69 @@ export const positionMmToKm = (position: number) => { export const calcOperationalPointsToDisplay = ( operationalPoints: OperationalPointType[], - isProportionnal: boolean, + isProportional: boolean, zoom: number -) => { - if (isProportionnal) { - const result = operationalPoints.reduce( - (acc, nextOp) => { - let op = { ...acc[acc.length - 1] }; - const newOp = { ...nextOp, display: true, styles: {} }; - const diff = positionMmToKm(nextOp.position - op.position); - if (diff * 8 * zoom >= BASE_OP_HEIGHT) { - acc.push(newOp); - op = nextOp; - } else { - newOp.display = false; - acc.push(newOp); - } - return acc; - }, - [{ ...operationalPoints[0], display: true, styles: {} } as StyledOperationalPointType] - ); +): StyledOperationalPointType[] => { + // For proportional display, we only display points that do not overlap with + // the last displayed point: + if (isProportional && operationalPoints.length >= 0) { + // We start with the first operational point: + const result: StyledOperationalPointType[] = [ + { ...operationalPoints[0], display: true, styles: {} }, + ]; + let lastDisplayedOP = result[0]; + + // We iterate through all points, and only add them if they don't collide + // with the last visible point: + for (let i = 1, l = operationalPoints.length; i < l; i++) { + const op = operationalPoints[i]; + const diff = positionMmToKm(op.position - lastDisplayedOP.position); + const display = diff * 8 * zoom >= BASE_OP_HEIGHT; + result.push({ + ...op, + display, + styles: {}, + }); - const lastElement = operationalPoints[operationalPoints.length - 1]; - if (result[result.length - 1] !== lastElement) { - result.push(lastElement); + if (display) lastDisplayedOP = result[i]; } - const secondLastIndex = result.length - 2; - if (secondLastIndex >= 0) { - const secondLastElement = result[secondLastIndex]; - const lastDiff = positionMmToKm(lastElement.position - secondLastElement.position); - if (lastDiff * 8 * zoom < BASE_OP_HEIGHT) { - result.splice(secondLastIndex, 1); + // In the end, to make sure the last point is visible, if it's not, we swap + // it with the last visible item: + const lastItem = result[result.length - 1]; + if (!lastItem.display) { + const lastVisibleItem = result.findLast((op) => op.display); + if (lastVisibleItem) { + lastVisibleItem.display = false; + lastItem.display = true; } } - result.shift(); + return result; - } else { - return operationalPoints as StyledOperationalPointType[]; + } + // For non-proportional display, we always display all the operational points: + else { + return operationalPoints.map((op) => ({ ...op, styles: {}, display: true })); } }; export const operationalPointsHeight = ( operationalPoints: StyledOperationalPointType[], zoom: number, - isProportionnal: boolean + isProportional: boolean ) => { return operationalPoints.map((op, index) => { const nextOp = operationalPoints[index + 1]; if (!nextOp) { - op.styles = { height: `${BASE_OP_HEIGHT}px` }; - return op; + return { ...op, styles: { height: `${BASE_OP_HEIGHT}px` } }; } - if (isProportionnal) { - op.styles = { - height: `${positionMmToKm(nextOp.position - op.position) * zoom * 8}px`, + if (isProportional) { + return { + ...op, + styles: { height: `${positionMmToKm(nextOp.position - op.position) * zoom * 8}px` }, }; - return op; } else { - op.styles = { height: `${BASE_OP_HEIGHT * zoom}px` }; - return op; + return { ...op, styles: { height: `${BASE_OP_HEIGHT * zoom}px` } }; } }); }; @@ -83,12 +86,12 @@ export const getOperationalPointsWithPosition = ( })); }; -export const getScales = (ops: OperationalPoint[], isProportionnal: boolean, zoomY: number) => { +export const getScales = (ops: OperationalPoint[], isProportional: boolean, zoomY: number) => { const scales = ops.slice(0, -1).map((point, i) => { return { from: point.position, to: ops[i + 1].position, - ...(!isProportionnal + ...(!isProportional ? { size: BASE_OP_HEIGHT * zoomY } : { coefficient: ((1000 / BASE_KM_HEIGHT) * 1000) / zoomY }), }; diff --git a/ui-manchette/src/types.ts b/ui-manchette/src/types.ts index 0bbeb590..4931c666 100644 --- a/ui-manchette/src/types.ts +++ b/ui-manchette/src/types.ts @@ -1,4 +1,4 @@ -import React from 'react'; +import { type CSSProperties } from 'react'; export type PathProperties = { electrifications?: { @@ -84,7 +84,7 @@ export type ArrayElement; export type StyledOperationalPointType = OperationalPointType & { - styles?: React.CSSProperties; + styles?: CSSProperties; display?: boolean; }; @@ -114,7 +114,6 @@ export type ProjectPathTrainResult = { positions: number[]; times: number[]; }[]; -} & { /** Departure time of the train */ departure_time: string; /** Rolling stock length in mm */