Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

front: add waypoint menu in manchette #9398

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions front/public/locales/en/simulation.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@
},
"trainList": "Train list",
"waiting": "Loading...",
"waypointMenu": {
"hide": "Hide this OP"
},
"waypointsPanel": {
"name": "name",
"secondaryCode": "CH",
Expand Down
3 changes: 3 additions & 0 deletions front/public/locales/fr/simulation.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@
},
"trainList": "Liste des trains",
"waiting": "Chargement en cours…",
"waypointMenu": {
"hide": "Masquer ce PR"
},
"waypointsPanel": {
"name": "nom",
"secondaryCode": "CH",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useRef, useState } from 'react';
import { useMemo, useRef, useState } from 'react';

import { KebabHorizontal } from '@osrd-project/ui-icons';
import { Manchette } from '@osrd-project/ui-manchette';
Expand All @@ -7,10 +7,12 @@ import { ConflictLayer, PathLayer, SpaceTimeChart } from '@osrd-project/ui-space
import type { Conflict } from '@osrd-project/ui-spacetimechart';

import type { OperationalPoint, TrainSpaceTimeData } from 'applications/operationalStudies/types';
import WaypointMenu from 'common/OSRDMenu';
import type { WaypointsPanelData } from 'modules/simulationResult/types';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be named OSRDMenu? It's a bit confusing to rename on import when there are no naming conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


import SettingsPanel from './SettingsPanel';
import ManchetteMenuButton from '../SpaceTimeChart/ManchetteMenuButton';
import useWaypointMenu from '../SpaceTimeChart/useWaypointMenu';
import WaypointsPanel from '../SpaceTimeChart/WaypointsPanel';

type ManchetteWithSpaceTimeChartProps = {
Expand All @@ -29,9 +31,11 @@ const ManchetteWithSpaceTimeChartWrapper = ({
waypointsPanelData,
conflicts = [],
}: ManchetteWithSpaceTimeChartProps) => {
const [heightOfManchetteWithSpaceTimeChart] = useState(DEFAULT_HEIGHT);
const manchetteWithSpaceTimeChartRef = useRef<HTMLDivElement>(null);

const [heightOfManchetteWithSpaceTimeChart] = useState(DEFAULT_HEIGHT);
const [showSettingsPanel, setShowSettingsPanel] = useState(false);
const [settings, setSettings] = useState({ showConflicts: false });
const [waypointsPanelIsOpen, setWaypointsPanelIsOpen] = useState(false);

const { manchetteProps, spaceTimeChartProps, handleScroll } = useManchettesWithSpaceTimeChart(
Expand All @@ -41,8 +45,25 @@ const ManchetteWithSpaceTimeChartWrapper = ({
selectedTrainScheduleId
);

const [showSettingsPanel, setShowSettingsPanel] = useState(false);
const [settings, setSettings] = useState({ showConflicts: false });
const waypointMenuData = useWaypointMenu(
manchetteWithSpaceTimeChartRef,
waypointsPanelData?.filteredWaypoints,
waypointsPanelData?.setFilteredWaypoints
);

const manchettePropsWithWaypointMenu = useMemo(
() => ({
...manchetteProps,
operationalPoints: manchetteProps.operationalPoints.map((op) => ({
...op,
onClick: (id: string, ref: HTMLDivElement | null) => {
waypointMenuData.handleWaypointClick(id, ref);
},
})),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can be simplified to:

onClick: waypointMenuData.handleWaypointClick,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point, done

activeOperationalPointId: waypointMenuData.activeOperationalPointId,
}),
[manchetteProps, waypointMenuData]
);

return (
<div className="manchette-space-time-chart-wrapper">
Expand All @@ -64,7 +85,19 @@ const ManchetteWithSpaceTimeChartWrapper = ({
style={{ height: `${heightOfManchetteWithSpaceTimeChart}px` }}
onScroll={handleScroll}
>
<Manchette {...manchetteProps} />
<Manchette {...manchettePropsWithWaypointMenu}>
{waypointMenuData.menuPosition && (
<WaypointMenu
menuRef={waypointMenuData.menuRef}
items={waypointMenuData.menuItems}
style={{
width: '305px',
top: waypointMenuData.menuPosition.top,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this to a CSS file?

Copy link
Contributor Author

@SharglutDev SharglutDev Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require a div wrapper as the width it's not always the same when calling the menu. I'm not a huge fan of that. Or you prefer passing a className prop instead of style ? How would you do ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't .manchette .osrd-menu work? If not, I'd suggest adding a className prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, you're right, done

left: waypointMenuData.menuPosition.left,
}}
/>
)}
</Manchette>
<div
className="space-time-chart-container"
style={{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React, { useRef, useState } from 'react';

import { EyeClosed } from '@osrd-project/ui-icons';
import { useTranslation } from 'react-i18next';

import type { OperationalPoint } from 'applications/operationalStudies/types';
import type { OSRDMenuItem } from 'common/OSRDMenu';
import useModalFocusTrap from 'utils/hooks/useModalFocusTrap';
import useOutsideClick from 'utils/hooks/useOutsideClick';

const SPACETIME_CHART_HEADER_HEIGHT = 40;
const WAYPOINT_MENU_OFFSET = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be a CSS margin instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


const useWaypointMenu = (
manchetteWrapperRef: React.RefObject<HTMLDivElement>,
filteredWaypoints?: OperationalPoint[],
setFilteredWaypoints?: (waypoints: OperationalPoint[]) => void
) => {
const { t } = useTranslation('simulation');

const [menuPosition, setMenuPosition] = useState<{ top: number; left: number }>();
const [activeOperationalPointId, setActiveOperationalPointId] = useState<string>();

const menuRef = useRef<HTMLDivElement>(null);

const closeMenu = () => {
setMenuPosition(undefined);
setActiveOperationalPointId(undefined);
};

useOutsideClick(menuRef, closeMenu);
useModalFocusTrap(menuRef, closeMenu);

const menuItems: OSRDMenuItem[] = [
{
title: t('waypointMenu.hide'),
icon: <EyeClosed />,
onClick: () => {
setMenuPosition(undefined);
setActiveOperationalPointId(undefined);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could call closeMenu here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (filteredWaypoints && setFilteredWaypoints) {
setFilteredWaypoints(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is racy, because another update to filteredWaypoints could be pending. Instead, setFilteredWaypoints can take a callback which mutates the current value:

setFilteredWaypoints((filteredWaypoints) => filteredWaypoints.filter());

As a bonus, this removes the need to pass filteredWaypoints as a prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I know but as it requires to pass the setFilteredWaypoints not as a standard function but as a SetStateAction, it needed to be changed everywhere we pass this props (but only two spots actually) and I was lazy 🙃 But you're right I should do that, done :)

filteredWaypoints.filter((waypoint) => waypoint.id !== activeOperationalPointId)
);
}
},
},
];

const handleWaypointClick = (id: string, ref: HTMLDivElement | null) => {
if (!ref || !manchetteWrapperRef.current) return;
const position = ref.getBoundingClientRect();
const manchetteWrapperPosition = manchetteWrapperRef.current.getBoundingClientRect();

// The position of the clicked waypoint is relative to the viewport so we need to
// substract the position of the manchetteWrapper to get the accurate position
setMenuPosition({
top:
position.bottom -
manchetteWrapperPosition.top +
SPACETIME_CHART_HEADER_HEIGHT -
WAYPOINT_MENU_OFFSET,
left: position.left - manchetteWrapperPosition.left,
});
setActiveOperationalPointId(id);
};

return { menuRef, menuPosition, menuItems, activeOperationalPointId, handleWaypointClick };
};

export default useWaypointMenu;
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@
.manchette {
overflow-y: auto;
overflow-x: hidden;

// TODO : remove this two styles when https://github.com/OpenRailAssociation/osrd-ui/issues/654 is fixed
.op {
height: 2rem;
line-height: 1.5rem;
padding-block: 0.1875rem 0.3125rem;

&::after {
bottom: 0.9844rem;
}
}
}

.space-time-chart-container {
Expand Down
Loading