-
Notifications
You must be signed in to change notification settings - Fork 46
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: add rolling stock categories #10790
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #10790 +/- ##
==========================================
- Coverage 82.56% 81.62% -0.94%
==========================================
Files 1087 1095 +8
Lines 108129 110398 +2269
Branches 742 742
==========================================
+ Hits 89272 90114 +842
- Misses 18815 20242 +1427
Partials 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9d72c64
to
135feea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR. Left some comments. I'm a bit confused on some AC, maybe there need to be some extra discussions with @maelysLeratRosso and/or @thibautsailly regarding the "requiredness" of the primary category :)
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorForm.tsx
Show resolved
Hide resolved
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Show resolved
Hide resolved
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
30dcfac
to
f9ddfe1
Compare
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
f9ddfe1
to
44cff13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm and tested, great job !
You can resolve the last comment when fixed (and don't forget to squash)
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
front/src/modules/rollingStock/components/RollingStockEditor/RollingStockEditorFormHelpers.tsx
Outdated
Show resolved
Hide resolved
3be1f7d
to
588dc79
Compare
588dc79
to
25d7e41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! But can you enhance the E2E tests?
For the "Create a new rolling stock" test:
- Verify that the primary category is checked and disabled in other categories
- Check additional categories and verify that they are selected
- Verify the categories after rolling stock creation
For the "Duplicate and modify a rolling stock" test:
- Modify the primary category
- Select additional categories
- Verify that the categories are updated after saving
0ff0bc6
to
7bc0cf2
Compare
front: add rolling stock category types and api processing Signed-off-by: Alice K. <[email protected]> front: add rolling stock category editor component Signed-off-by: Alice K. <[email protected]> front: add rolling stock category in card details Signed-off-by: Alice K. <[email protected]> front: add rolling stock category translations Signed-off-by: Alice K. <[email protected]> front: add rolling stock category to e2e tests Signed-off-by: Alice Khoudli <[email protected]>
Signed-off-by: Alice K. <[email protected]>
7bc0cf2
to
c82e229
Compare
Signed-off-by: Alice Khoudli <[email protected]>
Signed-off-by: Alice Khoudli <[email protected]>
8213de8
to
bf9ae66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job !
LGTM for e2e tests ✅
Close #10691
Depend on #10701
Behavior and translations should be fine, but the css could still use a little improvement ^^