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

feat: add OneKey on device selection screen #29610

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

Akaryatrh
Copy link
Contributor

@Akaryatrh Akaryatrh commented Jan 9, 2025

Description

Adding OneKey to the list of hardware devices on device selection page. Also, account label is now named "OneKey via Trezor".

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/accounts-planning/issues/762

Manual testing steps

  1. Open account list dropdown, click on Add Account or Hardware Wallet, then click on Add Hardware Wallet
  2. Unlock your OneKey device, select Onekey on the device list, click continue. Select your OneKey device on OS pop-up
  3. Select one or multiple accounts from the list and validate
  4. Check that accounts are listed as "OneKey x" on the account list.

Screenshots/Recordings

Before

Enregistrement.de.l.ecran.2025-01-13.a.15.20.49.mp4

After

Enregistrement.de.l.ecran.2025-01-13.a.15.36.08.mp4

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

github-actions bot commented Jan 9, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [e9a1c9c]
Page Load Metrics (1941 ± 134 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33025961866451217
domContentLoaded142425701914283136
load147825851941280134
domInteractive26233655225
backgroundConnect76125178
firstReactRender1787472412
getState666242010
initialActions00000
loadScripts101020341432239115
setupStore66419199
uiStartup178429052242322155

@metamaskbot
Copy link
Collaborator

Builds ready [ab9e7c2]
Page Load Metrics (1589 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint37118121536286137
domContentLoaded14301797156110249
load14391814158910852
domInteractive21176463818
backgroundConnect573282210
firstReactRender15102503316
getState5401094
initialActions01000
loadScripts1017132711479646
setupStore690182311
uiStartup16072409192920598

@metamaskbot
Copy link
Collaborator

Builds ready [ab9e7c2]
Page Load Metrics (1589 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint37118121536286137
domContentLoaded14301797156110249
load14391814158910852
domInteractive21176463818
backgroundConnect573282210
firstReactRender15102503316
getState5401094
initialActions01000
loadScripts1017132711479646
setupStore690182311
uiStartup16072409192920598

@metamaskbot
Copy link
Collaborator

Builds ready [31e7a91]
Page Load Metrics (1703 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1549186817028541
domContentLoaded1507185716829043
load1512186117039345
domInteractive257340168
backgroundConnect65719167
firstReactRender16101392713
getState46716199
initialActions01000
loadScripts1077138412398641
setupStore787192210
uiStartup17522578195616881

@Akaryatrh Akaryatrh marked this pull request as ready for review January 13, 2025 14:46
@metamaskbot
Copy link
Collaborator

Builds ready [70c0d6f]
Page Load Metrics (1679 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26219221623324156
domContentLoaded1536188616508340
load1540190616799546
domInteractive24195463919
backgroundConnect595312512
firstReactRender1698452612
getState45312115
initialActions01000
loadScripts1139142312366833
setupStore68211168
uiStartup17432410192715373

@metamaskbot
Copy link
Collaborator

Builds ready [70c0d6f]
Page Load Metrics (1679 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26219221623324156
domContentLoaded1536188616508340
load1540190616799546
domInteractive24195463919
backgroundConnect595312512
firstReactRender1698452612
getState45312115
initialActions01000
loadScripts1139142312366833
setupStore68211168
uiStartup17432410192715373

@metamaskbot
Copy link
Collaborator

Builds ready [fa990f1]
Page Load Metrics (1752 ± 115 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31324871618464223
domContentLoaded143624731731234112
load144524841752239115
domInteractive21181514321
backgroundConnect48418189
firstReactRender1694432914
getState455232010
initialActions00000
loadScripts10572029129620498
setupStore65912136
uiStartup172027042072254122

@metamaskbot
Copy link
Collaborator

Builds ready [d96856f]
Page Load Metrics (1708 ± 90 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14672161170117785
domContentLoaded14402115166916378
load14652166170818790
domInteractive2486412010
backgroundConnect6282376029
firstReactRender1781372411
getState54814147
initialActions00000
loadScripts10471609123914268
setupStore67415199
uiStartup169227261977277133

@metamaskbot
Copy link
Collaborator

Builds ready [d96856f]
Page Load Metrics (1708 ± 90 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14672161170117785
domContentLoaded14402115166916378
load14652166170818790
domInteractive2486412010
backgroundConnect6282376029
firstReactRender1781372411
getState54814147
initialActions00000
loadScripts10471609123914268
setupStore67415199
uiStartup169227261977277133

@Akaryatrh Akaryatrh added the needs-qa Label will automate into QA workspace label Jan 14, 2025
@Akaryatrh Akaryatrh requested review from a team as code owners January 20, 2025 21:28
@metamaskbot
Copy link
Collaborator

Builds ready [b3f66f0]
Page Load Metrics (1602 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint13901868160414871
domContentLoaded13771847158014971
load13821872160215273
domInteractive1997362010
backgroundConnect65823147
firstReactRender1578312211
getState46215167
initialActions01000
loadScripts10061410118512962
setupStore64819178
uiStartup158024321832218104

@metamaskbot
Copy link
Collaborator

Builds ready [efa40a4]
Page Load Metrics (1768 ± 101 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14882405177320699
domContentLoaded14832374174920799
load148724141768211101
domInteractive258139188
backgroundConnect57321209
firstReactRender1674342210
getState44811126
initialActions01000
loadScripts10531851129818287
setupStore66010126
uiStartup167626662056210101
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 112 Bytes (0.00%)
  • ui: 7.46 KiB (0.09%)
  • common: -16 Bytes (-0.00%)

@@ -309,7 +304,7 @@ class ConnectHardwareForm extends Component {
event: MetaMetricsEventName.AccountAdded,
properties: {
account_type: MetaMetricsEventAccountType.Hardware,
account_hardware_type: metricDeviceName,
account_hardware_type: deviceName,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not be deviceName but hardware type here. But, as discussed with MetaMask PM, to prevent issues in metrics, we'll cover this one through a different issue #29777

@metamaskbot
Copy link
Collaborator

Builds ready [f9e9fc8]
Page Load Metrics (2554 ± 505 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint263451920321243597
domContentLoaded1530449425311049504
load1542452525541051505
domInteractive25362958641
backgroundConnect46527157
firstReactRender17146593517
getState56422189
initialActions01000
loadScripts108236151934855411
setupStore6331494
uiStartup1763512929831295622
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 112 Bytes (0.00%)
  • ui: 7.32 KiB (0.09%)
  • common: -16 Bytes (-0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [a0a95c2]
Page Load Metrics (1683 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint23620051490496238
domContentLoaded14631875167012560
load14721903168313063
domInteractive2599442010
backgroundConnect44813105
firstReactRender15105452914
getState4499105
initialActions01000
loadScripts10191431122811254
setupStore690202713
uiStartup171025961976213102
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 152 Bytes (0.00%)
  • ui: 7.82 KiB (0.10%)
  • common: 8 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [de3f4bc]
Page Load Metrics (1765 ± 87 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14782062176818187
domContentLoaded14682027174817584
load14772068176518187
domInteractive2410745199
backgroundConnect64318126
firstReactRender1575372311
getState45419189
initialActions01000
loadScripts10561504128314268
setupStore65312136
uiStartup16302314201819593
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 152 Bytes (0.00%)
  • ui: 7.82 KiB (0.10%)
  • common: 8 Bytes (0.00%)

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Haven't finished reviewing it, will continue tomorrow

Comment on lines +17 to +25
<svg
width={width}
fill={color}
className={className}
aria-label={ariaLabel}
viewBox="60 0 520 288"
xmlns="http://www.w3.org/2000/svg"
>
<path d="M272.329 177.513C266.429 177.513 261.011 176.119 256.074 173.331C251.137 170.542 247.224 166.693 244.335 161.783C241.445 156.812 240 151.205 240 144.961C240 138.778 241.445 133.232 244.335 128.321C247.224 123.351 251.137 119.471 256.074 116.683C261.011 113.894 266.429 112.5 272.329 112.5C278.289 112.5 283.707 113.894 288.584 116.683C293.52 119.471 297.403 123.351 300.233 128.321C303.122 133.232 304.567 138.778 304.567 144.961C304.567 151.205 303.122 156.812 300.233 161.783C297.403 166.693 293.52 170.542 288.584 173.331C283.647 176.119 278.229 177.513 272.329 177.513ZM272.329 166.147C276.122 166.147 279.463 165.299 282.353 163.601C285.242 161.844 287.5 159.358 289.125 156.145C290.751 152.933 291.564 149.205 291.564 144.961C291.564 140.718 290.751 137.02 289.125 133.868C287.5 130.655 285.242 128.2 282.353 126.503C279.463 124.806 276.122 123.957 272.329 123.957C268.536 123.957 265.165 124.806 262.215 126.503C259.325 128.2 257.067 130.655 255.442 133.868C253.816 137.02 253.004 140.718 253.004 144.961C253.004 149.205 253.816 152.933 255.442 156.145C257.067 159.358 259.325 161.844 262.215 163.601C265.165 165.299 268.536 166.147 272.329 166.147Z"></path>
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved into a .svg file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to import directly the svg file as a react component, but for some reason it fails (need to declare svg module for typescript and even after it fails standardEntryPoints script).
I'd propose to try again on a separate PR and open a dedicated issue for this? (as multiple logos could use external svg files)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Label will automate into QA workspace team-hardware-wallets
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

3 participants