Likhith/webrel 795/refactor real account signup flow #9386
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
A production App ID was automatically generated for this PR. (log)
Click here to copy & paste above information. |
|
🚨 Lighthouse report for the changes in this PR:
Lighthouse ran with https://deriv-app-git-fork-likhith-deriv-likhith-webrel-795refac-77e5ba.binary.sx/ |
| import AddressDetails from '../address-details'; | ||
| import { isDesktop, isMobile, PlatformContext, TLocationList } from '@deriv/shared'; | ||
| import { FormikProps, FormikValues } from 'formik'; | ||
| import { isDesktop, isMobile, PlatformContext } from '@deriv/shared'; |
There was a problem hiding this comment.
you removed isDesktop and isMobile from the component itself,
should we refactor test accordingly?
There was a problem hiding this comment.
oh I see in the code of test it ok, so just remove here :)
There was a problem hiding this comment.
Here it is required because the DesktopWrapper and MobileWrapper are still making use of them
|
|
||
| beforeEach(() => { | ||
| (isDesktop as jest.Mock).mockReturnValue(true); | ||
| (isMobile as jest.Mock).mockReturnValue(false); |
There was a problem hiding this comment.
remove please these lines
There was a problem hiding this comment.
Here it is required because the DesktopWrapper and MobileWrapper are still making use of them
| const new_store_config = { | ||
| ...store, | ||
| ui: { | ||
| ...store.ui, | ||
| is_mobile: true, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
| const new_store_config = { | |
| ...store, | |
| ui: { | |
| ...store.ui, | |
| is_mobile: true, | |
| }, | |
| }; | |
| store.ui.is_mobile = true, |
Could you check it? I think we can just change the value without creating new store
There was a problem hiding this comment.
We can do it. I feel using immutable data is better
|
|
||
| it('should render AddressDetails component not svg', async () => { | ||
| mock_props.is_svg = false; | ||
| const new_props = { ...mock_props, is_svg: false }; |
There was a problem hiding this comment.
why do we need to create new object?
There was a problem hiding this comment.
I feel using immutable data is better as we know where the object has changed
| ] as TLocationList[]; | ||
|
|
||
| render(<AddressDetails {...mock_props} />); | ||
| const new_store_config = { |
There was a problem hiding this comment.
same here.
maybe we can just do the creation of store in beforeEach?
in this case we can avoid creating new objects and just change properties directly
There was a problem hiding this comment.
There are global settings available - mock_props, store, provider_value. I am modifying the ones that I need to simulate the test scenario
|
|
||
| export type TInputFieldValues = Record<string, string>; | ||
|
|
||
| export type TAddressDetailFormProps = { |
There was a problem hiding this comment.
is it really common types?
as I see we use it only for the component itself and for tests.
maybe we should keep it in the components file?
for using types in tests we can extract them with React.ComponentProps<typeof TAddressDetailFormProps>
There was a problem hiding this comment.
I'm using in spec file to type the FormikValue interface
packages/core/src/Stores/ui-store.js
Outdated
| is_account_switcher_disabled: computed, | ||
| is_mobile: computed, | ||
| is_tablet: computed, | ||
| is_desktop: computed, |
There was a problem hiding this comment.
please add new properties alphabetically :)
packages/stores/src/mockStore.ts
Outdated
| sub_section_index: 0, | ||
| toggleReadyToDepositModal: jest.fn(), | ||
| is_tablet: false, | ||
| is_desktop: false, |
There was a problem hiding this comment.
same here, please, alphabetically :)
| is_reports_visible: boolean; | ||
| is_language_settings_modal_on: boolean; | ||
| is_mobile: boolean; | ||
| is_desktop: boolean; |
| initialValues={props.value} | ||
| validate={handleValidate} | ||
| validateOnMount | ||
| onSubmit={(values, actions) => { |
There was a problem hiding this comment.
Would it be more readable if we move this logic to separate function?
| has_real_account, | ||
| ...props | ||
| }: TAddressDetails) => { | ||
| const { is_appstore } = React.useContext(PlatformContext); |
There was a problem hiding this comment.
Flag has been removed
likhith-deriv
requested review from
ali-hosseini-deriv and
amina-deriv
as code owners
* 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 * 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 * 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
|
Kudos, SonarCloud Quality Gate passed!
|
4 participants
Changes:
Please provide a summary of the change.
Screenshots:
Please provide some screenshots of the change.