osrd icon indicating copy to clipboard operation
osrd copied to clipboard

front: update timestop table

Open Synar opened this issue 1 year ago • 16 comments

Close #9352

A few points (name of the CI column, width of each column, text boldness and color in the ch column) remain open in the issue, and will be patched in this PR as soon as they are clarified. All other aspects should be implemented.

Synar avatar Oct 17 '24 14:10 Synar

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

Codecov Report

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

Project coverage is 39.74%. Comparing base (f7f1991) to head (701cc5c). Report is 3 commits behind head on dev.

Files with missing lines Patch % Lines
...c/modules/timesStops/hooks/useTimeStopsColumns.tsx 0.00% 50 Missing :warning:
front/src/modules/timesStops/TimesStopsOutput.tsx 0.00% 3 Missing :warning:
front/src/modules/timesStops/TimesStops.tsx 0.00% 2 Missing :warning:
...ons/operationalStudies/views/SimulationResults.tsx 0.00% 1 Missing :warning:

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #9375      +/-   ##
============================================
- Coverage     39.77%   39.74%   -0.03%     
  Complexity     2270     2270              
============================================
  Files          1302     1302              
  Lines         99565    99593      +28     
  Branches       3282     3282              
============================================
- Hits          39599    39584      -15     
- Misses        58034    58077      +43     
  Partials       1932     1932              
Flag Coverage Δ
core 75.06% <ø> (ø)
editoast 73.52% <ø> (-0.06%) :arrow_down:
front 10.19% <0.00%> (-0.01%) :arrow_down:
gateway 2.19% <ø> (ø)
osrdyne 3.28% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 86.71% <ø> (ø)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 17 '24 14:10 codecov-commenter

The last commit hardcode the line breaks and thus split the translations in 2, but I would rather revert this change

Synar avatar Oct 18 '24 15:10 Synar

Done ^^

Synar avatar Oct 23 '24 10:10 Synar

The name of the output table should be added: image

theocrsb avatar Oct 23 '24 14:10 theocrsb

The name of the output table should be added

I was not sure whether the name was included in the table and thus the desired changes, but I don't mind adding it

Synar avatar Oct 23 '24 14:10 Synar

The name of the output table should be added

I was not sure whether the name was included in the table and thus the desired changes, but I don't mind adding it

Thibaut is a bit busy right now so I don't want to bother him for that, but for now I have added a title in line with the information and style already on the page

Synar avatar Oct 23 '24 15:10 Synar

Good catch on the font size thanks, I did not think to check it

Synar avatar Oct 23 '24 15:10 Synar

I can't see the CI anymore when I try to modify a train

theocrsb avatar Oct 24 '24 06:10 theocrsb

I can't see the CI anymore when I try to modify a train

Good catch thanks, fixed, mb

Synar avatar Oct 24 '24 08:10 Synar

Is the size of the CI column as expected? image

theocrsb avatar Oct 28 '24 10:10 theocrsb

Is the size of the CI column as expected? image

I believe so, see thibaut comment on this in the issue. There should be a title tag to let you read the rest by hovering

Synar avatar Oct 28 '24 10:10 Synar

Thibaut said CI values have a maximum of 35 characters for now I think we don't have 35 characters. https://github.com/OpenRailAssociation/osrd/issues/9352#issuecomment-2416941708

theocrsb avatar Oct 28 '24 13:10 theocrsb

Maximum, not minimum

All columns but CI have a fixed width based on (...) • CI values have a maximum of 35 characters for now. (...) • When the viewport is too small to display the whole CI value, a title tag should carry over. (...)

I have reduced the max width of the CI column btw, though this will be pushed later. I discussed with thibaut and the title and units are fine as is, but we are going to make the durations right aligned (the times will stay left aligned, both for ease of comparison). The headers in English, as well as a couple of French ones, will use abbreviations with title tags with the full values in order to both make column narrowers and reduce the height of the header. Finally we may change the font of the min/100km unit, or otherwise find a way to make this column narrower.

Synar avatar Oct 28 '24 13:10 Synar

The fact that 0% values are hidden was already a hard coded behavior (in 2 different places, so clearly intentional). I don't understand why this is the case. I will discuss this with baptiste as soon as he's back, as well as other questions about margins displaying

Which column is the : with no seconds in?

Synar avatar Oct 29 '24 08:10 Synar

The fact that 0% values are hidden was already a hard coded behavior (in 2 different places, so clearly intentional). I don't understand why this is the case. I will discuss this with baptiste as soon as he's back, as well as other questions about margins displaying

Which column is the : with no seconds in?

Ok. In Requested Arrival Time

theocrsb avatar Oct 29 '24 08:10 theocrsb

I will remove the margins changes from this PR, to keep its scope more contained, and add them to another PR later (so until then the 0% will stay hidden, sorry).

I can not reproduce your bug about times being cut. Perhaps the column was simply too narrow? I can change the input table column width to make sure everything fits well.

Synar avatar Oct 29 '24 15:10 Synar