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: split the ScenarioView into 2 components #8866

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Conversation

clarani
Copy link
Contributor

@clarani clarani commented Sep 13, 2024

closes #8867

See commits

  • create a new component ScenarioDescription which contains all the info of the Scenario
  • create a new component ScenarioBody which contains the main content of the ScenarioView once we have a timetable, a scenario and an infra

- create a new ScenarioContent component which is rendered only if a scenario, a timetable and an infra exist
- modify useScenarioData so scenario, timetable and infra are arguments of the hook
@clarani clarani requested a review from a team as a code owner September 13, 2024 15:37
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 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 420 lines in your changes missing coverage. Please review.

Project coverage is 36.93%. Comparing base (df44230) to head (a456303).
Report is 18 commits behind head on dev.

Files with missing lines Patch % Lines
...nalStudies/components/Scenario/ScenarioContent.tsx 0.00% 247 Missing and 1 partial ⚠️
...tudies/components/Scenario/ScenarioDescription.tsx 0.00% 108 Missing and 1 partial ⚠️
...ations/operationalStudies/hooks/useScenarioData.ts 0.00% 41 Missing ⚠️
...applications/operationalStudies/views/Scenario.tsx 0.00% 13 Missing ⚠️
...plications/operationalStudies/hooks/useScenario.ts 0.00% 8 Missing and 1 partial ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #8866      +/-   ##
============================================
- Coverage     37.02%   36.93%   -0.09%     
  Complexity     2211     2211              
============================================
  Files          1255     1258       +3     
  Lines        114227   114476     +249     
  Branches       3182     3184       +2     
============================================
- Hits          42294    42285       -9     
- Misses        70042    70298     +256     
- Partials       1891     1893       +2     
Flag Coverage Δ
core 74.84% <ø> (ø)
editoast 72.41% <ø> (-0.04%) ⬇️
front 14.89% <0.00%> (-0.01%) ⬇️
gateway 2.20% <ø> (ø)
osrdyne 2.60% <ø> (-0.11%) ⬇️
railjson_generator 87.49% <ø> (ø)
tests 86.37% <ø> (ø)

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.

@achrafmohye
Copy link
Contributor

achrafmohye commented Sep 16, 2024

🚀 Amazing PR! Thanks for the great work! 🙌
Don't know if necessary at this part of the refacto or at all
Adding memoization to: trainScheduleSummaries and projectedTrains (avoid re-sorting),
trainScheduleUsedForProjection (optimize lookups),
upsertTrainSchedules (prevent re-renders).
Should boost performance?!

@clarani
Copy link
Contributor Author

clarani commented Sep 16, 2024

Thanks for your review @achrafmohye, I think every thing is already memoized with useMemo or useCallback in useScenarioData, do you think I can do something else ?

@clarani
Copy link
Contributor Author

clarani commented Sep 17, 2024

I added some memoization (for removeTrains and toggleMicroMacroButton)

Copy link
Contributor

@Uriel-Sautron Uriel-Sautron left a comment

Choose a reason for hiding this comment

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

Excellent work thank you.
Tested

Copy link
Contributor

@achrafmohye achrafmohye 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, thanks for the great work 👍

Copy link
Contributor

@Math-R Math-R left a comment

Choose a reason for hiding this comment

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

Lgtm nice work ! Much cleaner like this :)

@clarani clarani added this pull request to the merge queue Sep 17, 2024
Merged via the queue into dev with commit 2d6138c Sep 17, 2024
23 checks passed
@clarani clarani deleted the cni/refacto-scenario branch September 17, 2024 15:42
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: small refacto of the ScenarioView
5 participants