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: update timestop table #9375

Merged
merged 3 commits into from
Oct 31, 2024
Merged

front: update timestop table #9375

merged 3 commits into from
Oct 31, 2024

Conversation

Synar
Copy link
Contributor

@Synar Synar commented Oct 17, 2024

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 Synar requested a review from a team as a code owner October 17, 2024 14:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 17, 2024

⚠️ 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 ⚠️
front/src/modules/timesStops/TimesStopsOutput.tsx 0.00% 3 Missing ⚠️
front/src/modules/timesStops/TimesStops.tsx 0.00% 2 Missing ⚠️
...ons/operationalStudies/views/SimulationResults.tsx 0.00% 1 Missing ⚠️

❗ 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%) ⬇️
front 10.19% <0.00%> (-0.01%) ⬇️
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.

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

@Synar Synar force-pushed the ali/update-timestop-table branch 2 times, most recently from 523825d to 2204cd8 Compare October 18, 2024 15:12
@Synar
Copy link
Contributor Author

Synar commented Oct 18, 2024

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

@Synar Synar force-pushed the ali/update-timestop-table branch from 2204cd8 to 6a892ea Compare October 18, 2024 16:05
@Synar Synar requested review from clarani and axrolld October 18, 2024 16:06
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

  • Can you set the correct background color ? (behind the table)
  • the 3 first columns are not in the right order (see the mockup)

image

@Synar Synar requested review from clarani and a team and removed request for axrolld October 22, 2024 10:17
@Synar
Copy link
Contributor Author

Synar commented Oct 23, 2024

Done ^^

@Synar Synar force-pushed the ali/update-timestop-table branch 3 times, most recently from c832944 to 0958ed6 Compare October 23, 2024 13:06
Copy link
Contributor

@theocrsb theocrsb left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I've posted a few comments

@theocrsb
Copy link
Contributor

theocrsb commented Oct 23, 2024

The name of the output table should be added:
image

@Synar
Copy link
Contributor Author

Synar commented Oct 23, 2024

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

Copy link
Contributor

@theocrsb theocrsb left a comment

Choose a reason for hiding this comment

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

The font size is 14 everywhere except the header, which has a font size of 12px

@Synar
Copy link
Contributor Author

Synar commented Oct 23, 2024

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
Copy link
Contributor Author

Synar commented Oct 23, 2024

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

@Synar Synar force-pushed the ali/update-timestop-table branch from 0958ed6 to ec49a3b Compare October 23, 2024 16:15
@theocrsb
Copy link
Contributor

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

@clarani clarani self-assigned this Oct 24, 2024
@Synar
Copy link
Contributor Author

Synar commented Oct 24, 2024

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

Good catch thanks, fixed, mb

@Synar Synar force-pushed the ali/update-timestop-table branch 5 times, most recently from 417795b to 8e3a731 Compare October 25, 2024 14:14
@Synar Synar requested a review from theocrsb October 25, 2024 16:42
@theocrsb
Copy link
Contributor

theocrsb commented Oct 28, 2024

Is the size of the CI column as expected?
image

@Synar
Copy link
Contributor Author

Synar commented Oct 28, 2024

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

@theocrsb
Copy link
Contributor

Thibaut said CI values have a maximum of 35 characters for now
I think we don't have 35 characters.
#9352 (comment)

@Synar
Copy link
Contributor Author

Synar commented Oct 28, 2024

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.

Copy link
Contributor

@theocrsb theocrsb left a comment

Choose a reason for hiding this comment

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

I don't know if is this PR but we have a weird display in input table :
image
When we enter 0% we don't see anything displayed, but the array has kept a value.
image

And we see the two points
image

@Synar
Copy link
Contributor Author

Synar commented Oct 29, 2024

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?

@theocrsb
Copy link
Contributor

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

@Synar
Copy link
Contributor Author

Synar commented Oct 29, 2024

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.

Copy link
Contributor

@theocrsb theocrsb left a comment

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 want to change the style of the input panel. (which would solve the problem of unseen seconds).

@Synar Synar force-pushed the ali/update-timestop-table branch 2 times, most recently from 20721f3 to 1787ada Compare October 31, 2024 14:49
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

Almost good:

  • can you make the column CI take all the remaining space ? So the table fills the whole page ?
    image

  • can you make the input table fill the whole space too in its tab ?
    image

  • can you rebase and solve the conflicts ?

@Synar Synar force-pushed the ali/update-timestop-table branch from 1787ada to dab05c4 Compare October 31, 2024 15:40
Copy link
Contributor

@theocrsb theocrsb left a comment

Choose a reason for hiding this comment

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

LGTM and tested! Thank you for this PR

@Synar Synar force-pushed the ali/update-timestop-table branch from dab05c4 to 701cc5c Compare October 31, 2024 16:04
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

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

LGTM ✅

@Synar Synar added this pull request to the merge queue Oct 31, 2024
Merged via the queue into dev with commit f1e7a3c Oct 31, 2024
24 checks passed
@Synar Synar deleted the ali/update-timestop-table branch October 31, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

front: new output table
4 participants