-
Notifications
You must be signed in to change notification settings - Fork 44
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: update btn map in scenario page and convert rem to px in css map file #9798
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: theocrsb <theo_crosbie@yahoo.fr>
Signed-off-by: theocrsb <theo_crosbie@yahoo.fr>
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 #9798 +/- ##
==========================================
- Coverage 37.82% 37.79% -0.03%
==========================================
Files 994 989 -5
Lines 91126 91145 +19
Branches 1176 1169 -7
==========================================
- Hits 34466 34447 -19
- Misses 56206 56249 +43
+ Partials 454 449 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
||
.osrd-btn-map { | ||
color: var(--black100); | ||
width: 48px; |
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.
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.
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.
As you are re-writting these buttons, I think it would be better to do a bit of refacto :)
All these buttons are almost the same, the only things that change are the function onClick, the text in the span + its translation, the icon and the data-test-id.
You can create a generic component MapButton
which will take only these 4-5 props, and in MapButtons, render this new component with the correct props if we are using the newButtons.
If you do this, you won't need to modify all the existing Buttons
@@ -55,6 +56,7 @@ export default function MapButtons({ | |||
bearing, | |||
editorProps, | |||
viewPort: viewportProps, | |||
isNewButtons, |
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.
Set it to false by default, this way you can remove the props in the components which do not need the new buttons
isNewButtons, | |
isNewButtons = false, |
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.
Done
@@ -446,6 +446,7 @@ const Editor = () => { | |||
activeTool: toolAndState.tool, | |||
}} | |||
viewPort={viewport} | |||
isNewButtons={false} |
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.
You can remove this props if it's set by default to false
isNewButtons={false} |
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.
Done
You're right, I just proposed a refacto in my fixups :) |
Waiting for @thibautsailly feedback before to merge this PR
closes https://github.com/osrd-project/osrd-confidential/issues/670