-
Notifications
You must be signed in to change notification settings - Fork 0
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
Sergei / chore: migrate open-positions to tsx #40
Changes from 3 commits
cf05b3e
9673fe7
7b957ef
0814e8d
2285054
e4bda7b
5c13c20
2f5787d
7158294
993b611
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import React from 'react'; | |
import Text from '../text'; | ||
|
||
// Todo: Create a utility type for getting a range of float number. ex: FloatRange<0, 1> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sergei-deriv does this PR also implies that we address the TODO comment? 🙂 if yes, we can make a TFloatRange type according to an example in https://catchts.com/range-numbers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure, honestly :) But if you think I have to do this, I'll try. But for my PR I've got this type like this: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it’s out of this PR scope to handle the todo, But it will be nice if we add the generic @maryia-deriv suggested to |
||
type TRangeFloatZeroToOne = 0 | 0.1 | 0.2 | 0.3 | 0.4 | 0.5 | 0.6 | 0.7 | 0.8 | 0.9 | 1; | ||
export type TRangeFloatZeroToOne = 0 | 0.1 | 0.2 | 0.3 | 0.4 | 0.5 | 0.6 | 0.7 | 0.8 | 0.9 | 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sergei-deriv No need to export, Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've added this type in 'open-positions' component: |
||
|
||
type TProgressBarProps = { | ||
className?: string; | ||
|
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.
@sergei-deriv Not related to your PR, But since you have changed the condition I suggest you change it to the
Return Early
pattern.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.
I did it because with syntax "return should_show_sell && ( JSX )" this component returned JSX of False. But with my refactored code "return should_show_sell ? ( JSX ) : (JSX)" this component always returns JSX.
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.
@sergei-deriv You can use the
Return Early
pattern for better readability, It's still the same.It's just a suggestion tho 😅 Feel free to ignore it 🙇🏻
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.
It's really good advice, I fully agree with you. I've updated my commit.