Skip to content

Conversation

sergei-deriv
Copy link
Contributor

@sergei-deriv sergei-deriv commented Sep 13, 2023

Changes:

  • change color of balance to black
  • delete svg badge from account-info-wallets and account-switcher-wallets
  • change order of balance and badge
  • change title of account in dropdown

Screenshots:

Screenshot from design:
image

Screenshot from localhost:
image

@vercel
Copy link

vercel bot commented Sep 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Sep 15, 2023 6:26am

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

A production App ID was automatically generated for this PR. (log)

Click here to copy & paste above information.
- **PR**: [https://github.com/binary-com/deriv-app/pull/10051](https://github.com/binary-com/deriv-app/pull/10051)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-sergei-deriv-sergei-wall-1837remove-s-431679.binary.sx?qa_server=red.binaryws.com&app_id=38787
    - **Original**: https://deriv-app-git-fork-sergei-deriv-sergei-wall-1837remove-s-431679.binary.sx
- **App ID**: `38787`

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 21
🟧 Accessibility 75
🟢 Best practices 92
🟧 SEO 85
🟢 PWA 90

Lighthouse ran with https://deriv-app-git-fork-sergei-deriv-sergei-wall-1837remove-s-431679.binary.sx/

@coveralls
Copy link

Coverage Status

coverage: 10.891% (+0.09%) from 10.8% when pulling ec7b832 on sergei-deriv:sergei/wall-1837/remove-svg-badge into e532eb8 on binary-com:feature/wallets_with_traders_hub_2.

landing_company_name: wallet.landing_company_name?.replace('maltainvest', 'malta'),
/** Indicating whether the wallet is a maltainvest wallet. */
is_malta_wallet: wallet.landing_company_name === 'malta',
is_malta_wallet: wallet.landing_company_name === 'maltainvest',
Copy link
Contributor Author

@sergei-deriv sergei-deriv Sep 13, 2023

Choose a reason for hiding this comment

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

Note1: changed to maltainvest because replace function doesn't mutate original string and it's still maltainvest, not malta. Ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

Note2: we don't use Jurisdiction object from shared package because hooks package doesn't have shared package as dependency

const { ui } = useStore();
const { is_dark_mode_on } = ui;

const show_badge = wallet_account?.is_malta_wallet || wallet_account?.is_virtual;
Copy link
Contributor

Choose a reason for hiding this comment

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

@sergei-deriv add this logic to the parent of this component and make the badge an optional props so whenever you don't pass badge to it, it will be empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mahdiyeh-deriv What do you think if I'll add this logic inside of the WalletBadge component?

Copy link
Contributor Author

@sergei-deriv sergei-deriv Sep 14, 2023

Choose a reason for hiding this comment

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

@mahdiyeh-deriv I have a suggestion, let's add show_badge as prop to WalletBadge component, in result it will be really dumb component and logic to show/hide the badge will be in his parent? What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sergei-deriv yes, let's make the badge component a dummy one without any logic in that

const icon_type = is_virtual ? 'demo' : currency_config?.type;
const is_selected = is_active || linked_to?.some(account => account.loginid === active_account?.loginid);

const show_badge = is_malta_wallet || is_virtual;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mahdiyeh-deriv I'm not sure it's a good idea for this component (AccountSwitcherWalletItem), because this component needs to show every item in dropdown and we call it in AccountSwitcherWallet like this:

{dtrade_account_wallets?.map(account => (
                    <AccountSwitcherWalletItem
                        key={account.dtrade_loginid}
                        account={account}
                        closeAccountsDialog={closeAccountsDialog}
                    />
                ))}

So we just pass account and define to show or not the badge inside of the component. In your suggestion we need to add the same logic in AccountSwitcherWallet inside of the map method. Do you think is it better?
I understand what you mean, you want a dummy component as possible, but in my opinion this logic related to only this component. Also, we already have a lot of logic to define app_icon, icon_type, selected item or not, so I don't see any problem to add the logic to define to show the badge or not, because it depends on account which we already pass to the component.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine, keep it here and remove form the children component

@sergei-deriv sergei-deriv force-pushed the sergei/wall-1837/remove-svg-badge branch from cee3d6a to 16b833b Compare September 15, 2023 06:02
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants