front: add waypoint menu in manchette #9398

SharglutDev · 2024-10-21T06:53:55Z

Mockup :
https://www.sketch.com/s/a26d84be-5fb2-41e4-9e84-96d90f46a997/a/OeJV9yG

Note

Because of this issue : OpenRailAssociation/osrd-ui#654, some styles have been fixed here but should be removed when this issue will be closed.

codecov-commenter · 2024-10-21T06:56:11Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 88 lines in your changes missing coverage. Please review.

Project coverage is 39.05%. Comparing base (600370b) to head (589c8f7).

Files with missing lines	Patch %	Lines
...sult/components/SpaceTimeChart/useWaypointMenu.tsx	0.00%	53 Missing and 1 partial ⚠️
...WithSpaceTimeChart/ManchetteWithSpaceTimeChart.tsx	0.00%	31 Missing and 1 partial ⚠️
front/src/common/OSRDMenu.tsx	0.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #9398      +/-   ##
============================================
- Coverage     39.09%   39.05%   -0.05%     
  Complexity     2267     2267              
============================================
  Files          1308     1309       +1     
  Lines         99164    99245      +81     
  Branches       3312     3313       +1     
============================================
- Hits          38771    38756      -15     
- Misses        58430    58525      +95     
- Partials       1963     1964       +1

Flag	Coverage Δ
core	`74.93% <ø> (ø)`
editoast	`71.96% <ø> (-0.06%)`	⬇️
front	`10.31% <0.00%> (-0.02%)`	⬇️
gateway	`2.50% <ø> (ø)`
osrdyne	`3.30% <ø> (ø)`
railjson_generator	`87.49% <ø> (ø)`
tests	`86.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: SharglutDev <p.filimon75@gmail.com>

- When clicking on a waypoint in the manchette, display a menu with some actions - Only action for now is to hide the waypoint in the manchette Signed-off-by: SharglutDev <p.filimon75@gmail.com>

emersion

Overall I'm not a huge fan of having the menu outside of ui-manchette. IMHO the positioning and styling of the menu belongs to osrd-ui.

I'm especially concerned about the positioning logic which is quite fragile: it hardcodes a lot of assumptions about the manchette, and desynchronizes when elements move around. Example bug: zoom in the manchette, open the menu, and then scroll. This is just one example, other interactions may break the menu alike.

emersion · 2024-10-22T14:13:50Z

front/src/common/OSRDMenu.tsx

@@ -9,10 +9,11 @@ export type OSRDMenuItem = {
 type OSRDMenuProps = {
 menuRef: React.RefObject<HTMLDivElement>;
 items: OSRDMenuItem[];
+ style?: React.CSSProperties;


If this is exposed to allow callers to position the menu, should we instead add a position property? That would deter callers from (ab)using style for things which should stay in CSS files.

We also need to set the width, it's not always the same depending of who's calling the menu. I made the call to pass this props instead of pass width and position. You would prefer that ?

emersion · 2024-10-22T14:15:44Z

...ules/simulationResult/components/ManchetteWithSpaceTimeChart/ManchetteWithSpaceTimeChart.tsx

@@ -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';


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

emersion · 2024-10-22T14:24:33Z

...ules/simulationResult/components/ManchetteWithSpaceTimeChart/ManchetteWithSpaceTimeChart.tsx

+ menuRef={waypointMenuData.menuRef}
+ items={waypointMenuData.menuItems}
+ style={{
+ width: '305px',


Could we move this to a CSS file?

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. How would you do ?

emersion · 2024-10-22T14:26:37Z

...ules/simulationResult/components/ManchetteWithSpaceTimeChart/ManchetteWithSpaceTimeChart.tsx

+ onClick: (id: string, ref: HTMLDivElement | null) => {
+ waypointMenuData.handleWaypointClick(id, ref);
+ },


Nit: can be simplified to:

onClick: waypointMenuData.handleWaypointClick,

Oh good point, done

emersion · 2024-10-22T14:37:54Z

front/src/modules/simulationResult/components/SpaceTimeChart/useWaypointMenu.tsx

+ setMenuPosition(undefined);
+ setActiveOperationalPointId(undefined);
+ if (filteredWaypoints && setFilteredWaypoints) {
+ setFilteredWaypoints(


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.

emersion · 2024-10-22T14:47:49Z

front/src/modules/simulationResult/components/SpaceTimeChart/useWaypointMenu.tsx

+import useOutsideClick from 'utils/hooks/useOutsideClick';
+
+const SPACETIME_CHART_HEADER_HEIGHT = 40;
+const WAYPOINT_MENU_OFFSET = 2;


This could be a CSS margin instead.

emersion · 2024-10-22T14:48:11Z

front/src/modules/simulationResult/components/SpaceTimeChart/useWaypointMenu.tsx

+ setMenuPosition(undefined);
+ setActiveOperationalPointId(undefined);


We could call closeMenu here instead.

emersion · 2024-10-22T15:35:19Z

Actually, maybe this coule be used instead?

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_anchor_positioning/Using

SharglutDev changed the title ~~Pfn/front/add waypoint menu~~ front: add waypoint menu in manchette Oct 21, 2024

SharglutDev force-pushed the pfn/front/add-waypoint-menu branch from e76334c to 89acc1b Compare October 21, 2024 15:51

front: add style props to osrd menu

2b521c9

Signed-off-by: SharglutDev <p.filimon75@gmail.com>

SharglutDev force-pushed the pfn/front/add-waypoint-menu branch from 89acc1b to accef67 Compare October 21, 2024 20:16

front: add waypoint menu in manchette

589c8f7

- When clicking on a waypoint in the manchette, display a menu with some actions - Only action for now is to hide the waypoint in the manchette Signed-off-by: SharglutDev <p.filimon75@gmail.com>

SharglutDev force-pushed the pfn/front/add-waypoint-menu branch from accef67 to 589c8f7 Compare October 22, 2024 06:07

SharglutDev requested review from emersion and SarahBellaha October 22, 2024 07:36

SharglutDev marked this pull request as ready for review October 22, 2024 07:36

SharglutDev requested a review from a team as a code owner October 22, 2024 07:36

SharglutDev requested a review from Synar October 22, 2024 14:20

emersion reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

front: add waypoint menu in manchette #9398

front: add waypoint menu in manchette #9398

SharglutDev commented Oct 21, 2024 •

edited

Loading

codecov-commenter commented Oct 21, 2024 •

edited

Loading

emersion left a comment

emersion Oct 22, 2024

SharglutDev Oct 23, 2024

emersion Oct 22, 2024

emersion Oct 22, 2024

SharglutDev Oct 23, 2024

emersion Oct 22, 2024

SharglutDev Oct 23, 2024

emersion Oct 22, 2024

emersion Oct 22, 2024

emersion Oct 22, 2024

emersion commented Oct 22, 2024

		setMenuPosition(undefined);
		setActiveOperationalPointId(undefined);

front: add waypoint menu in manchette #9398

Are you sure you want to change the base?

front: add waypoint menu in manchette #9398

Conversation

SharglutDev commented Oct 21, 2024 • edited Loading

codecov-commenter commented Oct 21, 2024 • edited Loading

Codecov Report

emersion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emersion commented Oct 22, 2024

SharglutDev commented Oct 21, 2024 •

edited

Loading

codecov-commenter commented Oct 21, 2024 •

edited

Loading