Skip to content
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

chore(refactor): refactor mobile modal & fix phone dropdown #6294

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

mperdomo-bc
Copy link
Collaborator

No description provided.

@@ -91,8 +92,6 @@ class PhoneNumberBox extends React.Component {
defaultCountry={countryCode}
preferredCountries={['us', 'gb']}
css={['intl-tel-input', 'form-control']}
utilsScript='libphonenumber.js'
placeholder='555-555-5555'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the placeholder makes the component generate placeholders according to the selected country

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is even better, we will have exact format based on the country 👍

@@ -0,0 +1,43 @@
import React from 'react'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both MobileNumberAdd and MobileNumberChange are almost exactly the same, but due to them being used in different places sometimes and having custom analytics, I cannot merge them yet

@@ -136,7 +136,7 @@
"react-dropzone": "7.0.1",
"react-idle-timer": "5.2.0",
"react-intl": "5.24.4",
"react-intl-tel-input": "5.0.7",
"react-intl-tel-input": "8.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@@ -110,7 +109,7 @@ class PhoneNumberBox extends React.Component {
}

PhoneNumberBox.propTypes = {
countryCode: PropTypes.object.isRequired
countryCode: PropTypes.string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just curious was it really hard to migrate it to ts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to focus on the modals and then if necessary go through the components folder - always trying to balance the "make this repo better" with "this will be replaced not so far in the future"

@sstephanou-bc
Copy link

sstephanou-bc commented Feb 21, 2024

Logo
Checkmarx One – Scan Summary & Detailsabf6544f-c1b8-46eb-b968-be3093ddedfc

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM SSRF /packages/blockchain-wallet-v4-frontend/src/data/analytics/analytics.ts: 10 Attack Vector
MEDIUM SSRF /packages/blockchain-wallet-v4-frontend/src/middleware/analyticsMiddleware/analytics.ts: 10 Attack Vector

Fixed Issues

Severity Issue Source File / Package
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4/src/walletCrypto/index.ts: 127
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4/src/walletCrypto/utils.js: 99
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/data/bitpay/sagas.js: 32
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4/src/walletCrypto/utils.js: 122
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/hooks/useSardine/useSardine.ts: 2
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/hooks/useSardine/useSardine.ts: 41
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/hooks/useSardine/useSardine.ts: 2
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/data/modules/profile/sagas.ts: 1
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/data/modules/profile/sagas.ts: 624
LOW Use_of_Broken_or_Risky_Cryptographic_Algorithm /packages/blockchain-wallet-v4-frontend/src/data/modules/profile/sagas.ts: 1

@milan-bc
Copy link
Collaborator

I tested this on the staging but BE seems to fails when I try to add/update phone number with error 500. I did reported this, we should double check is all working before release

milan-bc
milan-bc previously approved these changes Feb 21, 2024
Copy link
Collaborator

@milan-bc milan-bc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@mperdomo-bc mperdomo-bc merged commit e06e027 into development Feb 27, 2024
2 checks passed
@mperdomo-bc mperdomo-bc deleted the refactor-mobile-modal branch February 27, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants