-
Notifications
You must be signed in to change notification settings - Fork 65
Sync timetable hover on train with space-time chart one #14850
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
base: dev
Are you sure you want to change the base?
Sync timetable hover on train with space-time chart one #14850
Conversation
9513702 to
d246925
Compare
| pathfindingHasFailed | ||
| ); | ||
|
|
||
| const hoveredPathId = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename this constant now it relates to path ids and train ids ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the chart calls it pathId, it’s the same TrainId in our code, so i renamed it hoveredTrainIdForChart which is clearer and doesn’t suggest two ID types.
achrafmohye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tested thanks
kmer2016
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM some nit
| dragging: boolean, | ||
| selectedTrainId?: TrainId | ||
| selectedTrainId?: TrainId, | ||
| hoveredTrainId?: TrainId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: just to clarify the source of the hover.
| hoveredTrainId?: TrainId | |
| hoveredTrainIdFromTimetable?: TrainId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! done
Signed-off-by: Mathieu <mathieu.coulibaly@sncf.fr>
d246925 to
c3a0634
Compare
close https://github.com/osrd-project/osrd-confidential/issues/1225