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

Add tabs for NGE integration #8006

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Add tabs for NGE integration #8006

merged 1 commit into from
Jul 24, 2024

Conversation

Uriel-Sautron
Copy link
Contributor

@Uriel-Sautron Uriel-Sautron commented Jul 10, 2024

closes #7537

@Uriel-Sautron Uriel-Sautron requested a review from a team as a code owner July 10, 2024 15:56
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

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.

lgtm (tested)

front/src/common/Tabs.tsx Outdated Show resolved Hide resolved
front/package.json Outdated Show resolved Hide resolved
@emersion
Copy link
Member

This needs a rebase against the dev branch.

Copy link
Contributor

@Yohh Yohh left a comment

Choose a reason for hiding this comment

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

LGTM 💪 tested

@codecov-commenter
Copy link

codecov-commenter commented Jul 11, 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 177 lines in your changes missing coverage. Please review.

Project coverage is 28.12%. Comparing base (cef8845) to head (7e3745e).

Files Patch % Lines
...cations/operationalStudies/views/v2/ScenarioV2.tsx 0.00% 79 Missing ⚠️
.../operationalStudies/components/MacroEditor/NGE.tsx 0.00% 59 Missing and 1 partial ⚠️
...operationalStudies/components/MicroMacroSwitch.tsx 0.00% 36 Missing and 1 partial ⚠️
front/src/common/Tabs.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    #8006      +/-   ##
============================================
- Coverage     28.16%   28.12%   -0.04%     
  Complexity     2120     2120              
============================================
  Files          1300     1302       +2     
  Lines        158791   158914     +123     
  Branches       3177     3179       +2     
============================================
- Hits          44718    44702      -16     
- Misses       112169   112306     +137     
- Partials       1904     1906       +2     
Flag Coverage Δ
core 75.42% <ø> (ø)
editoast 70.35% <ø> (-0.06%) ⬇️
front 9.97% <0.00%> (-0.02%) ⬇️
gateway 2.03% <ø> (ø)
railjson_generator 87.49% <ø> (ø)
tests 73.18% <ø> (ø)

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.

@emersion
Copy link
Member

emersion commented Jul 11, 2024

Discussed out-of-band with @clarani, we'll probably want to fix the ESLint complaints about the .js imports by sprinkling a few eslint-ignore comments (ESLint is getting confused by ?url):

  #141 329.3 /app/src/applications/operationalStudies/components/MacroEditor/NGE.tsx
  #141 329.3   3:21  warning  Unexpected use of file extension "js" for "@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/main.js?url"       import/extensions
  #141 329.3   3:21  warning  Unable to resolve path to module '@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/main.js?url'                import/no-unresolved
  #141 329.3   4:26  warning  Unexpected use of file extension "js" for "@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/polyfills.js?url"  import/extensions
  #141 329.3   4:26  warning  Unable to resolve path to module '@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/polyfills.js?url'           import/no-unresolved
  #141 329.3   5:24  warning  Unexpected use of file extension "js" for "@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/runtime.js?url"    import/extensions
  #141 329.3   5:24  warning  Unable to resolve path to module '@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/runtime.js?url'             import/no-unresolved
  #141 329.3   6:23  warning  Unable to resolve path to module '@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/styles.css?url'             import/no-unresolved
  #141 329.3   7:23  warning  Unexpected use of file extension "js" for "@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/vendor.js?url"     import/extensions
  #141 329.3   7:23  warning  Unable to resolve path to module '@osrd-project/netzgrafik-frontend/dist/netzgrafik-frontend/en/vendor.js?url'              import/no-unresolved

@emersion
Copy link
Member

The linter is unhappy, can be fixed by running yarn prettier --write <path>:

  #141 329.3 /app/src/applications/operationalStudies/views/v2/ScenarioV2.tsx
  #141 329.3   316:97  warning  Replace `{scenario.description}` with `⏎····················{scenario.description}⏎··················`  prettier/prettier

@Uriel-Sautron Uriel-Sautron force-pushed the usn/front/switch-osrd-nge branch from 9c3b915 to 05e536d Compare July 11, 2024 09:36
@Uriel-Sautron Uriel-Sautron requested a review from clarani July 11, 2024 09:53
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.

Discussed with Louis and Thibaut, this PR can not be merged like this otherwise the manageTrainSchedule and SimulationResults screens are very small, which may not be a good thing for the operational studies.

front/src/common/Tabs.tsx Outdated Show resolved Hide resolved
@Uriel-Sautron Uriel-Sautron requested a review from emersion July 16, 2024 12:21
@Uriel-Sautron Uriel-Sautron force-pushed the usn/front/switch-osrd-nge branch from 49314aa to 2a8502f Compare July 16, 2024 13:09
@emersion
Copy link
Member

Click on the "macro" tab, then the right arrow button: the sidebar opens, but it should just expand at the top instead.

out

@emersion
Copy link
Member

emersion commented Jul 16, 2024

TBH not sure this matters a lot and is worth fixing, we'll redesign the whole page soon enough… (e.g. we could just hide the arrow button in macro mode - maybe need to check with Thibault)

@emersion
Copy link
Member

Optional opinionated nits in terms of naming:

  • I'd find MicroMacroSwitch more descriptive than MicroMacroButtons personally
  • In general I prefer to name event callback props onXXX, e.g. onClickButton instead of handleClickButton. Maybe onChange would be even better?

@Uriel-Sautron Uriel-Sautron force-pushed the usn/front/switch-osrd-nge branch from 2a8502f to 150d2d7 Compare July 16, 2024 16:02
@Uriel-Sautron Uriel-Sautron requested a review from emersion July 16, 2024 16:02
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

LGTM! (but note that I wrote part of the NGE component)

@louisgreiner louisgreiner self-requested a review July 18, 2024 08:10
Copy link
Contributor

@louisgreiner louisgreiner left a comment

Choose a reason for hiding this comment

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

LGTM, thank you very much despite all the last-minute changes

front/package.json Show resolved Hide resolved
@Uriel-Sautron
Copy link
Contributor Author

Don't merge now we are waiting for the PO's decision

@louisgreiner
Copy link
Contributor

Can be merged, users will be informed of that specific feature

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.

LGTM (tested)

@Uriel-Sautron Uriel-Sautron requested a review from clarani July 22, 2024 07:45
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 & tested ✅ I just left 2 extra suggestions, you can fix and resolve the conversations on your own :) Don't forget to squash your commits

front: create NGE component

 - fix css for display nge integration

 - change function nge to arrow function

 - remove tabs componant and create buttons to switch between micro and macro components

 - remove flex-grow in scenario-timetable class, add class nge-iframe, remove classname in tabs component

 - create file for micro macro switch
@Uriel-Sautron Uriel-Sautron force-pushed the usn/front/switch-osrd-nge branch from 9d79c6a to 7e3745e Compare July 24, 2024 11:58
@Uriel-Sautron Uriel-Sautron added this pull request to the merge queue Jul 24, 2024
Merged via the queue into dev with commit ea0f6c9 Jul 24, 2024
20 checks passed
@Uriel-Sautron Uriel-Sautron deleted the usn/front/switch-osrd-nge branch July 24, 2024 12:27
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.

Being able to switch between the OSRD interface and the NGE interface
8 participants