feat: add keys option to the add shared wallet modal main page #1227

szymonmaslowski · 2024-06-18T11:41:47Z

Checklist

JIRA - <link>
Proper tests implemented
Screenshots added.

Proposed solution

Explain how does this PR solves the problem stated in JIRA ticket.
You can also enumerate different alternatives considered while approaching this task.

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

github-actions · 2024-06-18T12:12:52Z

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for 611bbdca

	passed	failed	skipped	flaky	total	result
Total	33	0	0	0	33	✅

szymonmaslowski · 2024-06-18T12:31:16Z

packages/shared-wallets/src/add-shared-wallet/creation-flow/SharedWalletCreationStore.tsx

-  StateConstantDataPart;
-
-export type StateSetup = MakeState<{
+const makeState = defineStateShape<{


All the changes in this file are unrelated to the main goal. Initially, I took a path of creating a dedicated store for the add modal main page so I created defineStateShape util so it could be reused in both stores. Later on I dropped the new store, but since I found those changes beneficial, I decided to keep them. I hope you think the same. However it that is not the case I am happy to revert.

szymonmaslowski · 2024-06-18T12:31:59Z

packages/shared-wallets/src/state-utils.ts

@@ -0,0 +1,23 @@
+type StateObject = Record<string, unknown>;


https://github.com/input-output-hk/lace/pull/1227/files#r1644386388

danielmain

Great job, Szymon. I like how you make the most of TypeScript generics. Just left small suggestions.

...llets/src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStarted.tsx

danielmain · 2024-06-19T08:21:19Z

...llets/src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStarted.tsx

+      ...commonKeysOptionCopies,
+      button: t('sharedWallets.addSharedWallet.getStarted.keysOption.button.copy'),
+    },
+    keysCopyToastText: 'Shared keys copied to clipboard',


should this not be placed in the translations?

...llets/src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStarted.tsx

…Props as discriminated union

sonarqubecloud · 2024-06-20T08:32:55Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

shawnbusuttil · 2024-06-20T09:27:35Z

packages/shared-wallets/src/add-shared-wallet/main-page-flow/AddSharedWalletMainPageFlow.tsx

+  sharedKeys?: string;
+};
+
+const makeCopyKeysToClipboard = (sharedKeys: string) => async () => {


Can be just copyKeysToClipboard?

I would prefer to stick to the make prefix as it indicates the function produces the target thing 🙏

Sorry maybe it's a bit too early in the day for me :D - why are we making a function for this and not directly invoking it in the component using it?

It is a good question 😛 I guess it is a matter of taste. I could inline it, define it in the component body right before the return, or do it the way I did it. I just find it cleaner this way 😉. But I am happy to move it to the component body. Would you like me to do it?

Yes please.

vetalcore · 2024-06-20T09:45:57Z

...src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStartedOption.tsx

-  <Flex flexDirection="column" p="$16" alignItems="center" justifyContent="space-between">
-    <Icon className={styles.icon} data-testid={`${testId}-icon`} />
+  <Flex flexDirection="column" p="$16" alignItems="center" justifyContent="space-between" className={styles.option}>
+    <Icon className={cn(styles.icon, { [`${styles.disabled}`]: disabled })} data-testid={`${testId}-icon`} />


Suggested change

<Icon className={cn(styles.icon, { [`${styles.disabled}`]: disabled })} data-testid={`${testId}-icon`} />

<Icon className={cn(styles.icon, { [styles.disabled]: disabled })} data-testid={`${testId}-icon`} />

vetalcore · 2024-06-20T09:46:09Z

...src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStartedOption.tsx

+        sx({
+          color: '$text_primary',
+        }),
+        { [`${styles.disabled}`]: disabled },


Suggested change

{ [`${styles.disabled}`]: disabled },

{ [styles.disabled]: disabled },

vetalcore · 2024-06-20T09:46:22Z

...src/add-shared-wallet/main-page-flow/SharedWalletGetStarted/SharedWalletGetStartedOption.tsx

-    <Text.Body.Small weight="$semibold" className={styles.description} data-testid={`${testId}-description`}>
+    <Text.Body.Small
+      weight="$semibold"
+      className={cn(styles.description, { [`${styles.disabled}`]: disabled })}


Suggested change

className={cn(styles.description, { [`${styles.disabled}`]: disabled })}

className={cn(styles.description, { [styles.disabled]: disabled })}

shawnbusuttil · 2024-06-20T10:04:00Z

packages/shared-wallets/src/add-shared-wallet/main-page-flow/AddSharedWalletMainPageFlow.tsx

+import { VFC } from 'react';
+import { SharedWalletGetStarted, SharedWalletGetStartedSharedProps } from './SharedWalletGetStarted';
+
+type AddSharedWalletMainPageFlowPrps = SharedWalletGetStartedSharedProps & {


Suggested change

type AddSharedWalletMainPageFlowPrps = SharedWalletGetStartedSharedProps & {

type AddSharedWalletMainPageFlowProps = SharedWalletGetStartedSharedProps & {

shawnbusuttil

Left small questions/suggestions.

szymonmaslowski self-assigned this Jun 18, 2024

Base automatically changed from feat/shared-wallets-package to main June 18, 2024 11:49

feat: add keys option to the add shared wallet modal main page

ff2bb90

szymonmaslowski force-pushed the feat/LW-10709-keys-option-in-add-shared-wallet branch from 4d20a26 to ff2bb90 Compare June 18, 2024 12:27

szymonmaslowski marked this pull request as ready for review June 18, 2024 12:28

szymonmaslowski requested review from a team as code owners June 18, 2024 12:28

szymonmaslowski commented Jun 18, 2024

View reviewed changes

danielmain suggested changes Jun 19, 2024

View reviewed changes

szymonmaslowski added 2 commits June 20, 2024 10:30

feat: add translation to shared keys coppied toast, define GetStarted…

4d26a75

…Props as discriminated union

Merge branch 'main' into feat/LW-10709-keys-option-in-add-shared-wallet

611bbdc

danielmain approved these changes Jun 20, 2024

View reviewed changes

shawnbusuttil reviewed Jun 20, 2024

View reviewed changes

vetalcore approved these changes Jun 20, 2024

View reviewed changes

shawnbusuttil reviewed Jun 20, 2024

View reviewed changes

oldGreg5 approved these changes Jun 20, 2024

View reviewed changes

szymonmaslowski merged commit ab257bd into main Jun 20, 2024
15 checks passed

szymonmaslowski deleted the feat/LW-10709-keys-option-in-add-shared-wallet branch June 20, 2024 13:40

pczeglik-iohk mentioned this pull request Jun 26, 2024

Feat add icons input-output-hk/lace-ui-toolkit#15

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add keys option to the add shared wallet modal main page #1227

feat: add keys option to the add shared wallet modal main page #1227

szymonmaslowski commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

szymonmaslowski Jun 18, 2024

szymonmaslowski Jun 18, 2024

danielmain left a comment

danielmain Jun 19, 2024

sonarqubecloud bot commented Jun 20, 2024

shawnbusuttil Jun 20, 2024

szymonmaslowski Jun 20, 2024

shawnbusuttil Jun 20, 2024

szymonmaslowski Jun 20, 2024

shawnbusuttil Jun 20, 2024

vetalcore Jun 20, 2024

vetalcore Jun 20, 2024

vetalcore Jun 20, 2024

shawnbusuttil Jun 20, 2024

shawnbusuttil left a comment

	<Icon className={cn(styles.icon, { [`${styles.disabled}`]: disabled })} data-testid={`${testId}-icon`} />
	<Icon className={cn(styles.icon, { [styles.disabled]: disabled })} data-testid={`${testId}-icon`} />

	{ [`${styles.disabled}`]: disabled },
	{ [styles.disabled]: disabled },

	className={cn(styles.description, { [`${styles.disabled}`]: disabled })}
	className={cn(styles.description, { [styles.disabled]: disabled })}

	type AddSharedWalletMainPageFlowPrps = SharedWalletGetStartedSharedProps & {
	type AddSharedWalletMainPageFlowProps = SharedWalletGetStartedSharedProps & {

feat: add keys option to the add shared wallet modal main page #1227

feat: add keys option to the add shared wallet modal main page #1227

Conversation

szymonmaslowski commented Jun 18, 2024 • edited Loading

Checklist

Proposed solution

Testing

Screenshots

github-actions bot commented Jun 18, 2024 • edited Loading

Allure Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jun 20, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnbusuttil left a comment

Choose a reason for hiding this comment

szymonmaslowski commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading