-
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
Use new speed limit tags endpoint #8266
Conversation
The previous list of speed-limit tag values from infra/RS has to be kept, but enriched (merged/deduplicated) with the new one. |
b8c9db2
to
df16db2
Compare
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 #8266 +/- ##
============================================
- Coverage 36.73% 36.71% -0.03%
Complexity 2163 2163
============================================
Files 1282 1282
Lines 119626 119659 +33
Branches 3218 3218
============================================
- Hits 43943 43928 -15
- Misses 73752 73800 +48
Partials 1931 1931
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks for the changes 🙏
There are some duplicate bugs on a local test on a small_infra copy (unlocked), docker, Firefox, WSL:
- edit infra, select existing speed-limit
- add an Additional speed_limit by tag
E30C
Then in operational studies (or STDCM) use that infra and create a train
- in the composition code list,
E30C
appears twice. - if using the search on that list it looks like when typing
E30C
, then backspace the list of E30C gets longer and longer (maybe a previously existing bug). - the list gets back to 2 duplicates only when stopping edit on the train then back to edit
The same goes for the selector in infra speed-limit editor, actually
front/src/common/SpeedLimitByTagSelector/useStoreDataForSpeedLimitByTagSelector.ts
Show resolved
Hide resolved
5a46c00
to
6c80b75
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.
Nice, one comment
6c80b75
to
3cff3f4
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
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.
Retested on the same cases, works fine, thank you for this 🙂
I didn't think of the speed-limit tag picker when displaying the infra: I don't know if it's better with all the values (including absent ones) or only the present ones... But both are OK IMO, so it's not a blocker for me.
EDIT: after discussion, let's use the same (extended) list for map layers, so current choices are good ✅
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 this PR, left some comments.
Also it's strange that we don't use the SpeedLimitByTagSelector
everywhere we need to select a speedlimit tag. I understand that it's not related to your PR but maybe you could create an enhancement to harmonize that ?
front/src/common/SpeedLimitByTagSelector/useStoreDataForSpeedLimitByTagSelector.ts
Outdated
Show resolved
Hide resolved
front/src/common/SpeedLimitByTagSelector/useStoreDataForSpeedLimitByTagSelector.ts
Outdated
Show resolved
Hide resolved
front/src/applications/editor/tools/rangeEdition/components/RangeEditionLeftPanel.tsx
Outdated
Show resolved
Hide resolved
front/src/common/SpeedLimitByTagSelector/useStoreDataForSpeedLimitByTagSelector.ts
Outdated
Show resolved
Hide resolved
- add new endpoint in useStoreDataForSpeedLimitByTagSelector and merge speed limit tags - use new speed limit tags endpoint in LayersModal, RangeEditionLeftPanel and MapSettingsSpeedLimits - add default speed limit tag in hook - ordered speed limit tags
454947f
to
65c51ef
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, good job !
Use the new endpoint
/speed_limit_tags
in front:Close #7975