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: fix blank page when train schedule with no arrival date #8140

Merged

Conversation

achrafmohye
Copy link
Contributor

Closes #8112

@achrafmohye achrafmohye requested a review from a team as a code owner July 19, 2024 13:17
@flomonster flomonster self-requested a review July 19, 2024 13:18
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2024

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

Codecov Report

Attention: Patch coverage is 55.55556% with 16 lines in your changes missing coverage. Please review.

Project coverage is 28.16%. Comparing base (dad4e39) to head (ff02097).
Report is 9 commits behind head on dev.

Files Patch % Lines
...le/components/TimetableV2/TimetableTrainCardV2.tsx 0.00% 13 Missing ⚠️
front/src/utils/date.ts 86.95% 0 Missing and 3 partials ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##                dev    #8140      +/-   ##
============================================
+ Coverage     28.12%   28.16%   +0.04%     
- Complexity     2112     2120       +8     
============================================
  Files          1296     1300       +4     
  Lines        158660   158820     +160     
  Branches       3164     3180      +16     
============================================
+ Hits          44621    44730     +109     
- Misses       112137   112183      +46     
- Partials       1902     1907       +5     
Flag Coverage Δ
core 75.42% <ø> (+0.14%) ⬆️
editoast 70.38% <ø> (-0.02%) ⬇️
front 9.99% <55.55%> (+0.01%) ⬆️
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.

@achrafmohye achrafmohye changed the title front: fix blank page when train schedule with no date front: fix blank page when train schedule with no arrival date Jul 19, 2024
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.

I have some structural remarks on this code (before the PR) and on the fix (the PR itself).
It's important to revamp the logic of this code to make it more robust.

@flomonster flomonster requested a review from theocrsb July 19, 2024 13:48
@achrafmohye achrafmohye force-pushed the ame/front/8112-fix-blank-page-train-schedule-with-no-date branch from 5f1404b to 4d4ad31 Compare July 23, 2024 09:55
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

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Some comments were not taken into account.

@achrafmohye achrafmohye force-pushed the ame/front/8112-fix-blank-page-train-schedule-with-no-date branch 2 times, most recently from 1d0fb12 to 0912716 Compare July 24, 2024 07:18
@flomonster flomonster self-requested a review July 24, 2024 08:53
Copy link
Contributor

@eckter eckter left a comment

Choose a reason for hiding this comment

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

LGTM, bug is fixed

@achrafmohye achrafmohye force-pushed the ame/front/8112-fix-blank-page-train-schedule-with-no-date branch from 0912716 to ff02097 Compare July 24, 2024 11:45
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

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

Looks okay.

Note

The date management requires a code refactoring.

The transformation process is weird, for example the train.startTime process is as follows 🤯:

  1. String ISO8601
  2. Dayjs obj
  3. String custom DD/MM/YYYY HH:mm:ss
  4. Date obj
  5. Dayjs obj
  6. String custom DD/MM HH:mm:ss

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, I created a enhancement to refacto this later : #8166

@achrafmohye achrafmohye added this pull request to the merge queue Jul 24, 2024
Merged via the queue into dev with commit 28c656e Jul 24, 2024
20 checks passed
@achrafmohye achrafmohye deleted the ame/front/8112-fix-blank-page-train-schedule-with-no-date branch July 24, 2024 14: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.

A train schedule breaks the frontend: dateTime.split is undefined
6 participants