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: new output table #9352

Closed
clarani opened this issue Oct 15, 2024 · 29 comments · Fixed by #9375
Closed

front: new output table #9352

clarani opened this issue Oct 15, 2024 · 29 comments · Fixed by #9375
Assignees
Labels
area:front Work on Standard OSRD Interface modules

Comments

@clarani
Copy link
Contributor

clarani commented Oct 15, 2024

Description and goal

Relates to https://github.com/osrd-project/osrd-confidential/issues/668

The current output table of the scenario page does not match the design system.
We should create a new one (see the mock-up below):

Image

The scope of this ticket is only to create the new output table (and not the buttons below). The table can not be modified (for the moment).

There are still 2 things to clarify with @thibautsailly: what should we display in the column "Reception sur signal fermé" and which title do we want for the new column that @RomainValls is going to add in his PR (#9275) (will it change the global display ?) ?

Acceptance criteria

The new output table is in the scenario page

@clarani clarani added the area:front Work on Standard OSRD Interface modules label Oct 15, 2024
@louisgreiner
Copy link
Contributor

louisgreiner commented Oct 15, 2024

Many notes:

  • is CI the right column name ? Since we display the name of the PR, or something else for the Waypoints (track offset). The CI actually refers to the identifier which is rather a unique number
  • do we want the CI column to be that big ? We could think on a way to resize this column, especially for smaller screens, and maybe display the full name on mouse hover of this cell. This column would have a lesser priority, and be resized dynamically in priority, before allowing to scroll on the whole table @thibautsailly
  • is the inconsistent coloring intended ? I guess it should be "one line beige, one line white" but lines 4 and 5 are both white
    Image
  • the IDs are not coherent, jumping from 13 to 38 for instance, but I guess it should be just incremental steps, not that kind of jumps ?

It would also be nice to have an example of Waypoint use in this table

@Castavo
Copy link
Contributor

Castavo commented Oct 15, 2024

Three others comments:

  • The operational study teams asked for a "track" column to be added, would now be the right time for that ?
  • "marge réelle" and "diff marge" columns should only be filled when there is a "marge théorique" value
  • The "marge théorique" is very often not expressed in percentages, and min/100km cannot be converted to percentages

@louisgreiner
Copy link
Contributor

As @Castavo said, "marge théorique" unit is both % and min/100km, depending on the margin chosen.

As mentioned in the Propositions section of the specifications issue, the input table would let the user select the margin type (as it is done in the RS editor) => imo, we then need the unit to be in the cell directly

@Synar Synar self-assigned this Oct 15, 2024
@Synar
Copy link
Contributor

Synar commented Oct 16, 2024

Right now the first line and columns are sticky, is this a behavior we want to keep? I didn't see it mentioned in the specs.

Also the behaviors seem to be pretty inline with the specs already, so just to be sure this issue is mostly just styling, correct?

@thibautsailly
Copy link

Yes this is mostly a restyling work.

@thibautsailly
Copy link

• CI is the term used in Last Minute Request for this field.
• All columns but CI have a fixed width based on the column title's width (the line return is part of the label, not a style) plus horizontal padding (L8 and R16). They float right, if I may.
• CI values have a maximum of 35 characters for now. Gaia cuts them at this value, this may be fixed in the future.
• When the viewport is too small to display the whole CI value, a title tag should carry over. When the viewport is very large, too bad. We won't spend time fixing this use case for this version, but in the next one.
• The inconsistent colouring is my fault, sorry. I can export the fixed version if you want.
• This is not the right time to add the track column

@thibautsailly
Copy link

Is there a way to adapt the column title of "marge théorique" depending on the unit used in order to display the unit actually used and not both?

@Synar
Copy link
Contributor

Synar commented Oct 16, 2024

For "important rows", the text of certain columns (index, ch) is not bold. Is this intentional? Are these the only two columns where text should not be bold?

Is there a way to adapt the column title of "marge théorique" depending on the unit used in order to display the unit actually used and not both?

As long as all rows always use the same type of margin, this should be possible. Is this guaranteed?

@Castavo
Copy link
Contributor

Castavo commented Oct 17, 2024

@thibautsailly

Is there a way to adapt the column title of "marge théorique" depending on the unit used in order to display the unit actually used and not both?

Both units may be used independently in a circulation... Maybe we should constrain that ? this is a hard requirement

@Synar
Copy link
Contributor

Synar commented Oct 17, 2024

* "marge réelle" and "diff marge" columns should only be filled when there is a "marge théorique" value

It seems to me that for every op point "marge théorique (s)" is always filled from the simulation result even if "marge théorique (weird units)" is not. So "marge réelle" and "diff marge" columns should already only be filled when there is a "marge théorique (s)" value, as they are are computed at the same time, but sometimes this will be the case without "marge théorique (weird units)" being filled in the first place. Is the behavior we want?

@louisgreiner
Copy link
Contributor

Also, I understand that we would have different column names (at least for CI) depending on the module it is called on. Is there more specificities ?

@louisgreiner
Copy link
Contributor

louisgreiner commented Oct 17, 2024

@Synar

"marge réelle" and "diff marge" columns should only be filled when there is a "marge théorique" value

It seems to me that for every op point "marge théorique (s)" is always filled from the simulation result even if "marge théorique (weird units)" is not. So "marge réelle" and "diff marge" columns should already only be filled when there is a "marge théorique (s)" value, as they are are computed at the same time, but sometimes this will be the case without "marge théorique (weird units)" being filled in the first place. Is the behavior we want?

I'm pretty sure that we don't want "marge théorique (s)" to be filled if no "marge théorique (weird units)" is filled. The fact that it is always filled is probably due to the data structure ? It doesn't make sense from a business perspective otherwise. It is either all margins, either none. Am I wrong ?

@Synar
Copy link
Contributor

Synar commented Oct 17, 2024

The specifications do not correspond to the mockup for the background color of important points.

un PR important, texte en gras et couleur d'arrière plan de la ligne en gris

I went with the mockup for now (not changing the background color for these lines, only making the text bold)

@louisgreiner
Copy link
Contributor

louisgreiner commented Oct 17, 2024

The specifications need to be updated, since it has been written with the older version (not yet restyled). For styling, you can refer to the mockup in priority (= ignore the grey background color)

@Synar
Copy link
Contributor

Synar commented Oct 17, 2024

I understand that each column's width is supposed to be fixed width, but I can't find a way to access the width of each column in the mockup. Also the time columns' widths seem insufficient to incorporate "J+1" text that I believe may be present?

@thibautsailly
Copy link

I understand that each column's width is supposed to be fixed width, but I can't find a way to access the width of each column in the mockup. Also the time columns' widths seem insufficient to incorporate "J+1" text that I believe may be present?

My comment about this wasn't clear I'm afraid, sorry.

All columns but CI have a fixed width based on the column title's width (the line return is part of the label, not a style) plus horizontal padding (L8 and R16). They float right, if I may.

You should not specify width for the columns, only margins around the column titles and force them to be displayed on two lines. The column width will depend on the label itself. All columns but index and CI are right aligned.

@Synar
Copy link
Contributor

Synar commented Oct 17, 2024

You should not specify width for the columns, only margins around the column titles and force them to be displayed on two lines. The column width will depend on the label itself. All columns but index and CI are right aligned.

So do you want the columns to only resize to adapt the header titles and not their content? Or should I let the columns adapt to the size of its content, only giving padding to the titles? Looking at the mock up I suppose the second option is correct?

@Synar
Copy link
Contributor

Synar commented Oct 17, 2024

Also the CH column has a different text color and in the mockup, and is not bolded for "important points". Is it intentional?

@Synar
Copy link
Contributor

Synar commented Oct 18, 2024

All columns but index and CI are right aligned.

To be clear, are you are talking about the entire columns or just the headers? Because the columns in the mockup seem to be left aligned

@Synar
Copy link
Contributor

Synar commented Oct 18, 2024

the line return is part of the label

How do we transpose this to different languages? Do we hardcode each line of the label for each language?

@Synar
Copy link
Contributor

Synar commented Oct 18, 2024

So React-Datasheet-Grid, the lib we are using for the table right now, really except its columns to be sized in a flex way based on min-width, max-width, basis, grow and shrink. Most of the columns before this issue used to use a fixed width.

I may have overlooked something, but I don't see a simple way to have the columns dynamically size themselves only based on the header length using that library without providing any hardcoded pixel base-width. How important is this aspect? I'm also not convinced I understand why we would want such a behavior in the first place.

@Synar
Copy link
Contributor

Synar commented Oct 22, 2024

@thibautsailly

@Synar
Copy link
Contributor

Synar commented Oct 23, 2024

Ok so for now I have reverted the split of the translations and most columns have fixed sizes

It would be nice though to better handle columns resizing with content size, but this might require changing our table library.

@anisometropie
Copy link
Contributor

@Synar @thibautsailly

Why do we give the same width to all columns ?

Some columns content is known and never change: CH, time columns, checkbox columns don’t need to be dynamic

@Synar
Copy link
Contributor

Synar commented Oct 23, 2024

@Synar @thibautsailly

Why do we give the same width to all columns ?

Some columns content is known and never change: CH, time columns, checkbox columns don’t need to be dynamic

Thanks. The size of the content does not change indeed, but the size of the header do (depending on the language), meaning we can't perfectly control on how many lines our header will be displayed. I personally don't think it's a huge deal though, but it's a departure from the mockup.

@anisometropie
Copy link
Contributor

@Synar @thibautsailly
The thing is this table has got so wide over time with all these new columns, we get a horizontal scroll, wihch we should really try to avoid.
Screenshot_20241023_180052

I feel we should try to focus on making columns as narrow as possible.
can we not trim some of the header text ? (do we really need the full “reception on signal closed” for example)

@Castavo
Copy link
Contributor

Castavo commented Oct 24, 2024

@anisometropie I think the goal of this issue is just styling for coherence, the improvement in usability will come later

@anisometropie
Copy link
Contributor

From what I see columns width was an integral part of the discussion.

Actually I think the requirement given by @thibautsailly would work, we just need to trim some of them

All columns but CI have a fixed width based on the column title's width (the line return is part of the label, not a style) plus horizontal padding (L8 and R16). They float right, if I may.
You should not specify width for the columns, only margins around the column titles and force them to be displayed on two lines. The column width will depend on the label itself. All columns but index and CI are right aligned.

@Castavo
Copy link
Contributor

Castavo commented Oct 31, 2024

This issue is kinda linked to this one: #9526

It should be done after this one, before dropping the "fiche train" component

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 a pull request may close this issue.

6 participants