Kate / DTRA-378 / TS migration: Multiplier components and containers in Trader package #26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kate-deriv
commented
Aug 30, 2023
| @@ -98,7 +98,7 @@ const Dialog = ({ | |||
| } | |||
| }; | |||
|
|
|||
| const validateClickOutside = () => dismissable || (has_close_icon && is_visible && is_closed_on_cancel); | |||
| const validateClickOutside = () => !!dismissable || !!(has_close_icon && is_visible && is_closed_on_cancel); | |||
There was a problem hiding this comment.
Function validateClickOutside is passed as an argument inside of useOnClickOutside hook. This hook is expecting function, which is returning boolean.
kate-deriv
commented
Aug 30, 2023
|
|
||
| return ( | ||
| <React.Fragment> | ||
| <Modal | ||
| id='dt_trade_parameters_mobile' | ||
| className='trade-params' | ||
| enableApp={enableApp} |
There was a problem hiding this comment.
<Modal /> is not expecting enableApp and disableApp props
kate-deriv
commented
Aug 30, 2023
| @@ -76,7 +78,7 @@ const TradeParamsMobile = observer(({ toggleModal }) => { | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| }, [stake_value]); | |||
|
|
|||
| const setSelectedAmount = (basis, stake) => { | |||
There was a problem hiding this comment.
Parameter basis isn't used inside of the function, but I didn't remove it because <AmountMobile /> is waiting for function with 2 params. Type of function inside of <AmountMobile /> is better not to change, because other components which are used <AmountMobile /> passed function with 2 params
kate-deriv
commented
Aug 30, 2023
| @@ -104,10 +106,10 @@ const TradeParamsMobile = observer(({ toggleModal }) => { | |||
| toggleModal={toggleModal} | |||
| amount_tab_idx={0} | |||
| setSelectedAmount={setSelectedAmount} | |||
| setAmountError={() => {}} | |||
There was a problem hiding this comment.
Made this param optional in order not to pass 'empty' function
kate-deriv
commented
Aug 30, 2023
| setState(applied_risk_management_state); | ||
| onClose(...args); | ||
| onClose(); |
There was a problem hiding this comment.
onClose function is actually a () => setDialogOpen(false) (passed as a props from risk-management-info.tsx), so there is no need in ...args
|
⏳ Generating Lighthouse report... |
kate-deriv
commented
Aug 31, 2023
| @@ -127,11 +127,14 @@ const Basis = observer( | |||
| } | |||
| ); | |||
|
|
|||
| type TAmountMobile = React.ComponentProps<typeof Basis> & { | |||
There was a problem hiding this comment.
We don't need all props from <Basis />
kate-deriv
commented
Aug 31, 2023
| @@ -314,8 +314,6 @@ const TradeParamsMobile = observer( | |||
| setAmountError={setAmountError} | |||
| stake_value={stake_value} | |||
| payout_value={payout_value} | |||
| basis={''} | |||
There was a problem hiding this comment.
Made this params optional
| const { name, value } = e.target; | ||
| const new_state = { ...state }; | ||
| new_state[name] = value; | ||
| new_state[name as 'take_profit' | 'stop_loss' | 'cancellation_duration'] = value; |
There was a problem hiding this comment.
Suggested change
| new_state[name as 'take_profit' | 'stop_loss' | 'cancellation_duration'] = value; | |
| new_state[name as keyof typeof new_state] = value; |
will this work?)
There was a problem hiding this comment.
No, it wouldn't :(
Reason: new_state was created based on state object, which type is
state: {
take_profit: string | undefined;
stop_loss: string | undefined;
has_take_profit: boolean;
has_stop_loss: boolean;
has_cancellation: boolean;
cancellation_duration: string;
}
So, some of the fields are boolean and a value (from e.target) is a string. And here comes an error, that string is not assignable to that type (which includes boolean). That is why I used union type
packages/trader/src/Modules/Trading/Components/Elements/Multiplier/cancel-deal-mobile.tsx
Outdated
Show resolved
Hide resolved
packages/trader/src/Modules/Trading/Components/Elements/Multiplier/cancel-deal-mobile.tsx
Outdated
Show resolved
Hide resolved
packages/trader/src/Modules/Trading/Containers/Multiplier/multiplier-options.tsx
Outdated
Show resolved
Hide resolved
kate-deriv
commented
Aug 31, 2023
| const stake_ref = React.useRef(amount); | ||
| const [stake_value, setStakeValue] = React.useState<string | number>(amount); | ||
| const [commission, setCommission] = React.useState<number | null>(); | ||
| const [stop_out, setStopOut] = React.useState<number | null>(); |
There was a problem hiding this comment.
commission and stop_out are passed as a props to <MultipliersInfo />, where there is a check if they exists (e.g. props.multiplier ?? ...). So it seems safe not to set default value as null, but to do it undefined. In this case there is no error with TTradeStore['onProposalResponse'], because there commission and stop_out can be number | null | undefined
maryia-deriv
approved these changes
Aug 31, 2023
henry-deriv
approved these changes
Sep 1, 2023
kate-deriv
merged commit eba9fff
into
kate/ts_migration_trader_package_5
2 of 3 checks passed
kate-deriv
added a commit
that referenced
this pull request
Oct 11, 2023
* fix: ts-migrate trade-params * fix: sonarcloud * fix: pull in changes from tech debt 2 package * fix: resolve comments * fix: move type to common prop types * fix: move type to common prop types * fix: move type to common prop types file * fix: resolve comments * Kate / DTRA-321 / TS migration of trade-params and trade-params-mobile (#6) * refactor: migrate trade params and started mobile version * refactor: ts migration of trade params mobile * chore: add nessasary prop * refactor: apply suggestions * chore: change todo text * refactor: add import * fix: circleCI error * fix: ts migrate trade-footer-extensions.jsx * fix: fix import * fix: remove progress-slider-stream since its not used * fix: resolve comments * fix: reset with master * fix: reset with master * fix: reset with master * fix: reset with master * fix: circleCI * fix: togglePositions folder * fix: dtra-346 marketisclosed and marketcountdowntimer migration * fix: bug * Maryia/dtra-270/TS migration: trade-store (#5) * chore: prepare for migration * chore: add more types to trade-store * chore: add more types to trade-store * chore: add more types to trade-store * chore: add more types to trade-store * chore: migrated trade-store to ts * chore: improve types in trade-store * fix: type * revert: hooks package installation * refactor: address review * fix: resolve comments * feat: add ts migartion of store * refactor: add prev changes * chore: empty commit * fix: add lost mocked * fix: resolve comments * Kate / DTRA-354 / Components/Form/Purchase files in Trader package (#21) * refactor: ts migartion of purchase files * refactor: remove duplicated types * refactor: apply suggestions * maryia/fix: sonarcloud warnings (#7) * fix: sonarcloud warnings * fix: Unexpected end of JSON input * fix: bug * fix: sonarcloud * fix: reorder props * fix: test cases * fix: coveralls * fix: coveralls * fix: this component doesnt exist anymore, hence test was also removed * henry/dtra-356/fix: ts-migration-digitsJSX (#24) * fix: initial commit * fix: ts migrate digits JSX * fix: small type change * fix: comment * chore: removed unused state * Maryia/dtra-355/Migrate ContractDrawer files to TS (#22) * feat: migrated swipeable-components to TS * feat: migrated market-closed-contract-overlay & index to TS * chore: migrated contract-drawer-card.tsx to ts * build: fix type * chore: migrated contract-drawer to ts * chore: fixed existing types in digits and logic * chore: sort types properties in alphabetical order * Kate/dtra 357/ts contract audit files (#23) * refactor: apply suggestion from prev pr * refactor: start ts migration of contract audit * chore: change comment * refactor: ts of contract audit item * refactor: ts migration of contract details * refactor: ts migration of contract history * refactor: add preprepared types * refactor: tests * chore: apply nit * refactor: apply suggestions * refactor: apply suggestions * chore: fix of sonar cloud * Maryia/dtra-373/remove localize from ContractCardHeader component (#25) * refactor: remove localize from contract-card-header * refactor: use Localize component instead of localize helper * build: install RTL deps in shared package * feat: merge previous tech debt branch * fix: sonar cloud codesmells * Merge branch kate/ts_migration_trader_package_4 into kate/ts_migration_trader_package_5 * Kate / DTRA-378 / TS migration: Multiplier components and containers in Trader package (#26) * refactor: ts of cancel deal and risk managment info * fix: types in dialog tsx * refactor: start ts of multiplier options * refactor: ts of mult amount modal * refactor: remove default val from basis component * fix: type of amount mobile * refactor: applied suggestions * refactor: ts of multiplier info (#28) * Henry/dtra 376/fix: ts migration positionsdrawer folder (#27) * fix: positions drawer folder * fix: coveralls * fix: where is my commit * fix: found commits * fix: file rename * fix: comments * fix: comments * fix: correct logic * fix: remove unused props * fix: types * refactor: remove code mells * fix: ts-migrate-populate header (#30) * Maryia/DTRA-377/TS migration: TradingDatePicker + TradingTimePicker + TimePicker + Dialog (#29) * chore: timepicker to ts * chore: trading timepicker to ts * chore: add types for TradingDatePicker & DatePicker * refactor: added default values * refactor: remove code small * chore: empty commit * fix: not using index as key (#31) * fix: ternary bug * fix: build TS errors (#32) * refactor: apply suggestions * refactor: change type of time * chore: empty commit * refactor: tests * fix: conflicts * fix: conflicts * fix: tests * fix: types in tests * fix: test extention * fix: add turbos to types back * fix: add backup for target name * fix: revert empty strings --------- Co-authored-by: Henry Hein <henry@regentmarkets.com> Co-authored-by: Maryia <103177211+maryia-deriv@users.noreply.github.com> Co-authored-by: henry-deriv <118344354+henry-deriv@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes:
Screenshots:
Please provide some screenshots of the change.