-
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
Likhith/webrel 780/migrate Email and password section #7
Conversation
Pull Request Test Coverage Report for Build 5878354849
💛 - Coveralls |
* @param {boolean} is_open - state to toggle the modal | ||
* @param {string} identifier_title - title of the identifier (e.g. Google, Facebook) | ||
* @param {Function} onClickSendEmail - function to send email to user | ||
* @returns {React.ReactNode} - returns jsx component |
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.
the PR is looking good to me, except of additional lines for explaining props in different components :)
but you can ignore it :)
packages/account/src/App.tsx
Outdated
<StoreProvider store={root_store}> | ||
<Routes /> | ||
<ResetTradingPassword /> | ||
<APIProvider> |
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.
@likhith-deriv @deriv/stores
uses @deriv/api
, APIProvider
should wrap StoreProvider
.
* @param {Function} onClose - function to close the modal | ||
* @param {boolean} is_open - state to toggle the modal | ||
* @param {string} identifier_title - title of the identifier (e.g. Google, Facebook) | ||
* @param {Function} onClickSendEmail - function to send email to user | ||
* @returns {React.ReactNode} - returns jsx component |
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.
In my opinion, this kind of documentation especially for components is a bit too much, It will get outdated very quickly over time. I would suggest instead using TSDoc in the types.
type TProps = {
/** Callback to be called once the dialog has been closed. */
onClose: () => void;
...
};
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 preferred documenting props because the type may not be in the same file as the component. When dev reds the component code they can see the prop description as well
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.
@likhith-deriv Prop types of a component are isolated to the component itself and ideally, we won't be using them anywhere else. I've seen in the CFD package we have a huge prop type file for all the components and then export everything from there, We shouldn't be doing that 😬
const { | ||
common: { is_from_derivgo }, | ||
client: { social_identity_provider, is_social_signup }, | ||
} = useStore(); |
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.
const { | |
common: { is_from_derivgo }, | |
client: { social_identity_provider, is_social_signup }, | |
} = useStore(); | |
const { common, client } = useStore(); | |
const { is_from_derivgo } = common; | |
const { social_identity_provider, is_social_signup } = client; |
@likhith-deriv It's easier to read in my opinion.
common: { is_from_derivgo }, | ||
client: { social_identity_provider, is_social_signup }, | ||
} = useStore(); | ||
const { send } = useVerifyEmail('request_email'); |
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.
@likhith-deriv Shouldn't we handle loading and error state too? 🤔
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.
We should. But I am unaware of what must be the flow when there is an error
setIsSendEmailModalOpen(true); | ||
} | ||
}; | ||
|
||
const onClickSendEmail = () => { | ||
WS.verifyEmail(email, 'request_email'); | ||
send(email); | ||
setIsUnlinkAccountModalOpen(false); | ||
setIsSendEmailModalOpen(true); |
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.
@likhith-deriv Shouldn't we do this once the call was successful?
const { | ||
traders_hub: { is_eu_user, financial_restricted_countries }, | ||
client: { social_identity_provider, is_social_signup }, | ||
} = useStore(); | ||
const { send: requestEmail } = useVerifyEmail('request_email'); | ||
const { send: resetPassword } = useVerifyEmail('reset_password'); |
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.
Same here.
} else { | ||
WS.verifyEmail(email, 'reset_password'); | ||
resetPassword(email); | ||
} | ||
setIsSentEmailModalOpen(true); |
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.
Same here.
}, | ||
}); | ||
setIdenifier(cfd_platform); | ||
|
||
setIdentifier(cfd_platform ?? ''); | ||
setIsSentEmailModalOpen(true); |
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.
Same here.
<DerivEmail email={email} /> | ||
<DerivPassword email={email} /> |
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.
@likhith-deriv I believe email
here is prop drilling, We can just get it from useStore
if I'm not mistaken 🤔
packages/account/src/App.tsx
Outdated
// TODO: Remove this once we have removed usage of WS in @deriv/account | ||
setWebsocket(WS); |
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.
@likhith-deriv You can remove it, It's been added to the core
package a while ago so other packages don't need to set it anymore.
<React.Fragment> | ||
<FormSubHeader title={getPlatformTitle()} /> | ||
<div className='account__passwords-wrapper'> | ||
{has_mt5_accounts && ( |
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.
@likhith-deriv I think we can break this into 2 separate components 🙏🏻
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.
Will refactor this
type TPasswordsPlatformProps = { | ||
has_dxtrade_accounts?: boolean; | ||
has_mt5_accounts?: boolean; | ||
}; |
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.
@likhith-deriv Seems to be prop drilling, We can get these values from client store (or even better, useFetch
) 🤔
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 is no prop drilling here. The props are being computed and set from Password
component to be used in Password-platform
component
⏳ Generating Lighthouse report... |
@@ -0,0 +1,64 @@ | |||
import React from 'react'; |
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.
File name has a typo in it 🚫
common: { is_from_derivgo }, | ||
client: { social_identity_provider, is_social_signup, email }, | ||
} = useStore(); | ||
const { send } = useVerifyEmail('request_email'); |
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.
useVerifyEmail
seems like it is not complete and needs some updates.
We should be able to WS.verifyEmail(email, 'reset_password');
reset password with it as well.
const onClickResendEmail = () => { | ||
WS.verifyEmail(email, 'request_email'); | ||
}; |
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.
Why?
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.
The function is not needed as it has been improved in refactoring
onClickSendEmail={onClickResendEmail} | ||
onClickSendEmail={() => send(email)} |
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.
isn't send(email)
and send(email, 'request_email') different?
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.
The send function is part of an instance of verifyEmail of type request_email
a644a57
into
shahzaib-deriv:KYC-accounts-package-TS-migration--test-coverage/sprint-8
* 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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#25) * refactor: remove localize from contract-card-header * refactor: use Localize component instead of localize helper * build: install RTL deps in shared package * fix: sonar cloud codesmells * fix: build TS errors (deriv-com#32) * refactor: tests * fix: more conflicts * chore: empty commit * maryia/fix: type issues on package 4 (deriv-com#41) * fix: type issues * chore: file change from package 3 * chore: keep small changes from package 3 * chore: removed unnecessary todo comment * refactor: apply suggestions * fix: types in tests * fix: test extention * fix: wallet file * fix: add turbos to types --------- 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>
* fix: ⚡ refactored Address-details component * fix: 🎨 failing testcases * refactor: ♻️ incorporated review comments * feat: 🔥 removed PlatformContext * fix: testcase failure * refactor: ♻️ incorporated review comments * refactor: ♻️ refactor component to remove unwanted code (deriv-com#44) * refactor: ♻️ refactor component to remove unwanted code * feat: removed commented code * test: ✅ added testcase * test: 🧪 added tests * refactor: ♻️ incorporated review comments * refactor: ♻️ incorporated review comments * refactor: ♻️ incorporated review comments * refactor: ♻️ migrated code to TS (deriv-com#45) * refactor: ♻️ migrated code to TS * refactor: ♻️ incorporated review comments * refactor: ♻️ incorporated review comments * Update packages/account/src/Types/common-prop.type.ts Co-authored-by: yauheni-deriv <103182683+yauheni-deriv@users.noreply.github.com> --------- Co-authored-by: yauheni-deriv <103182683+yauheni-deriv@users.noreply.github.com> * refactor: ⚡ refactored currency selector component (deriv-com#43) * refactor: ⚡ refactored currency selector component * fix: 🚨 lint errors * feat: incorporated store values * fix: 🎨 split code into small components * fix: 🎨 removed un-necessary array * refactor: ♻️ incorporated review comments * refactor: ♻️ incorporated review comments * resolved failing test cases * chore: incorporated review comment * fix: build issue * fix: build issue * ref: incorporated review comments * refactor: Modified code to contain hooks * refactor: Modified code to contain hooks * replaced test component * fix: eslint issues * fix: failing testcases * ref: Incorporated review somments * Likhith/webrel 780/migrate Email and password section (#7) * chore: migrated deriv-email and unlink component * chore: migrate password-platform to tsx * chore: migrate password-platform to tsx * chore: added missing test cases * ref: incorporated review comments * refactor: incorporated review comments * refactor: incorporated review comments * refactor: incorporated review comments * ref: Incorporated review somments * chore: config files ts migration and refactor (#8) * chore: config files ts migartion and refactor * refactor: test case for config func * fix: config typing * refactor: remove custom accounts residence type * refactor: review comment * chore: added test cases * fix: revert boolean in types * refactor: default value fields * refactor: files types, import react types * refactor: react types * fix: condition rule * fix: failing testcase --------- Co-authored-by: “yauheni-kryzhyk-deriv” <“yauheni@deriv.me”> * fix: incorporated review comments * fix: failing testcase * refactor: migrate form-fields file (#10) * refactor: migrate form-fields file * refactor: migrate form-fields file * ref: incorporated review comments * fix: failing testcase * fix: incorporated review comments * fix: incorporated review comments * fix: incorporated review comments * fix: resolved Sonar lint errors * fix: tslint issues * fix: review comments * fix: review comments * refactor: 🎨 incorporated types for test cases * fix: 🦺 incorporated review comments * fix: 🎨 incorporated ui-store-fix * fix: ⚰️ replaced deprecated hooks * fix: 🔥 unused dependency * Merge branch 'master' into KYC-accounts-package-TS-migration--test-coverage/sprint-8 * chore: added package-lock * fix: added value for mock-store --------- Co-authored-by: yauheni-deriv <103182683+yauheni-deriv@users.noreply.github.com> Co-authored-by: “yauheni-kryzhyk-deriv” <“yauheni@deriv.me”> Co-authored-by: Shahzaib <shahzaib@deriv.com>
* 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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#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 (deriv-com#28) * Henry/dtra 376/fix: ts migration positionsdrawer folder (deriv-com#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 (deriv-com#30) * Maryia/DTRA-377/TS migration: TradingDatePicker + TradingTimePicker + TimePicker + Dialog (deriv-com#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 (deriv-com#31) * fix: ternary bug * fix: build TS errors (deriv-com#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>
Changes: