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: fix displayed op on manchette #10334

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

Akctarus
Copy link
Contributor

@Akctarus Akctarus commented Jan 13, 2025

Set the weight of the main PRs (origin, arrival and vias) to 100.
For vias added by hand from the map, they were previously set to “null”, now they're set to 100, so they're displayed on the manchette.

Tests :

  • add a waypoint using the functionality, see it displayed on the basic cuff and GEV
  • add a waypoint by clicking on the map, and see it displayed on the headline and GEV.
  • check PR weights console.log(operationalPoints.weight) and verify that passage PRs have a weight of 100

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Jan 13, 2025
@Akctarus Akctarus self-assigned this Jan 13, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.59%. Comparing base (0ee63cf) to head (456612e).
Report is 39 commits behind head on dev.

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

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #10334      +/-   ##
==========================================
- Coverage   81.65%   81.59%   -0.06%     
==========================================
  Files        1065     1067       +2     
  Lines      105650   105541     -109     
  Branches      723      732       +9     
==========================================
- Hits        86265    86120     -145     
- Misses      19344    19380      +36     
  Partials       41       41              
Flag Coverage Δ
editoast 73.64% <ø> (-0.04%) ⬇️
front 89.30% <100.00%> (-0.05%) ⬇️
gateway 2.18% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.50% <ø> (ø)
tests 87.05% <ø> (ø)

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.

@Akctarus Akctarus marked this pull request as ready for review January 14, 2025 21:22
@Akctarus Akctarus requested a review from a team as a code owner January 14, 2025 21:22
@@ -20,7 +20,7 @@ import { formatData } from './helpers';
export type SpeedSpaceChartContainerProps = {
trainSimulation: SimulationResponseSuccess;
selectedTrainPowerRestrictions?: LayerData<PowerRestrictionValues>[];
pathProperties: PathPropertiesFormatted;
pathProperties?: PathPropertiesFormatted;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think it makes sense to have a speed space chart without path properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I fixed it

Copy link
Contributor

@Caracol3 Caracol3 left a comment

Choose a reason for hiding this comment

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

Works great ! Good job !
I just left one comment

@@ -35,7 +35,7 @@ export const upsertMapWaypointsInOperationalPoints = (
},
part: { track: step.track, position: step.offset },
position: positionOnPath,
weight: null,
weight: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid magic numbers and because it's also used in useGetProjectedTrainOperationalsPoints maybe we should use a const like 'HIGHEST_PRIORITY_WEIGHT' here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed !

@@ -15,38 +15,53 @@ export const upsertMapWaypointsInOperationalPoints = (
t: TFunction
): OperationalPoint[] => {
let waypointCounter = 1;
const HIGHEST_PRIORITY_WEIGHT = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be outside of the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
setFilteredOperationalPoints(operationalPointsWithUniqueIds);
setFilteredOperationalPoints(operationalPointsWithAllWaypoints);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should change this. Otherwise, we won't have any waypoint checked in the waypoint panel.
We should ask @Tguisnet if on linear mode of the manchette, we now want to view all points or just some of them if a weight superior than a specific number etc

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure where it comes from, but there is a white screen if we try to switch to linear mode in the manchette

Signed-off-by: Theo Macron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants