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.

Sign up for GitHub

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

Jump to bottom

feat(suite): Update passphrase modal #12076

Merged
merged 4 commits into from
May 15, 2024
Merged

feat(suite): Update passphrase modal #12076

merged 4 commits into from
May 15, 2024

Conversation

jvaclavik
Copy link
Contributor

@jvaclavik jvaclavik commented Apr 19, 2024 •
edited
Loading

Description

Related Issue

Resolve

Screenshots:

Default switch device modal:
image

Switching view only:
image

Create passphrase wallet:

1. type passphrase

image image

2. loading

image

3. unused wallet

image

4. open unused wallet
image

5. confirm passphrase
image

@jvaclavik jvaclavik force-pushed the view-only-passphrase-modal branch 2 times, most recently from 9a1a06f to bae5507 Compare April 19, 2024 13:07
jvaclavik
jvaclavik commented Apr 19, 2024
View reviewed changes
packages/suite/src/components/suite/modals/ModalSwitcher/DiscoveryLoaderLegacy.tsx
@@ -0,0 +1,26 @@
import styled from 'styled-components';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing except name is changed here

jvaclavik
jvaclavik commented Apr 19, 2024
View reviewed changes
...es/suite/src/components/suite/modals/ReduxModal/DeviceContextModal/PassphraseModalLegacy.tsx
@@ -0,0 +1,199 @@
import { useCallback, useState } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing except name is changed here

@jvaclavik jvaclavik force-pushed the view-only-passphrase-modal branch 4 times, most recently from 9aa9872 to dafcfd0 Compare April 26, 2024 13:36
@jvaclavik jvaclavik force-pushed the view-only-passphrase-modal branch from dafcfd0 to 4965f5a Compare May 9, 2024 14:48
jvaclavik
jvaclavik commented May 13, 2024
View reviewed changes
packages/components/src/components/Passphrase/PassphraseTypeCardLegacy.tsx
@@ -0,0 +1,433 @@
import { useState, useRef, useEffect, useCallback, ReactNode, ChangeEvent } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing except name is changed here

jvaclavik
jvaclavik commented May 13, 2024
View reviewed changes
packages/suite/src/components/suite/modals/ReduxModal/DeviceContextModal/PassphraseModal.tsx
}

// creating standard wallet here instead of showing dialog
onSubmit('');
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 creates standard wallet

peter-sanderson
peter-sanderson reviewed May 13, 2024
View reviewed changes
packages/suite/src/components/suite/layouts/SuiteLayout/SuiteLayout.tsx
@@ -111,6 +111,8 @@ export const SuiteLayout = ({ children }: SuiteLayoutProps) => {

const isAccountPage = !!selectedAccount;

useAppShortcuts();
Copy link
Contributor

@peter-sanderson peter-sanderson May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Us this right place for this? I think we have all the "global" staff in Main (I know we have 2, one for web and one for wallet) or Preloader. This should be just a layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's not global, when you are in onboarding you shouldn't be able to run all shortcuts.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 (Maybe lets add this info as a comment?)

@jvaclavik jvaclavik marked this pull request as ready for review May 14, 2024 15:31
peter-sanderson
peter-sanderson reviewed May 15, 2024
View reviewed changes
packages/components/src/components/Flex/Flex.tsx
@@ -36,16 +36,19 @@ export const flexAlignItems = [
export type FlexDirection = (typeof flexDirection)[number];
export type FlexJustifyContent = (typeof flexJustifyContent)[number];
export type FlexAlignItems = (typeof flexAlignItems)[number];
export type Flex = string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is anyway beneficial, but why not. One day we may figure out how to make it stricter.

peter-sanderson
peter-sanderson reviewed May 15, 2024
View reviewed changes
packages/components/src/components/Passphrase/PassphraseTypeCard.stories.tsx Outdated
// import {
// PassphraseTypeCardLegacy as PassphraseTypeCardComponent,
// PassphraseTypeCardLegacyProps as PassphraseTypeCardProps,
// } from './PassphraseTypeCardLegacy';
Copy link
Contributor

Choose a reason for hiding this comment

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

!

peter-sanderson
peter-sanderson reviewed May 15, 2024
View reviewed changes
packages/components/src/components/Passphrase/PassphraseTypeCardContent.tsx Outdated
`;

type PassphraseTypeCardContentProps = {
submitLabel: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think (from what I saw in the code) we prefer import from react and not use the whole React reference.

peter-sanderson
peter-sanderson reviewed May 15, 2024
View reviewed changes
packages/components/src/components/Passphrase/PassphraseTypeCardHeading.tsx Outdated
content={
<FormattedMessage
id="TR_HIDDEN_WALLET_TOOLTIP"
defaultMessage="Passphrases add a custom phrase (e.g. a word, sentence, or string of characters) to your recovery seed. This creates a hidden wallet; each hidden wallet can use its own passphrase. Your standard wallet will still be accessible without a passphrase."
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: do we need those defaultMessage in the code? We have them in message.jsom file so here it seems to me as unnecessary stuff that pollutes code.

peter-sanderson
peter-sanderson reviewed May 15, 2024
View reviewed changes
packages/suite/src/views/suite/SwitchDevice/DeviceItem/DeviceItem.tsx Outdated
@@ -123,6 +75,8 @@ export const DeviceItem = ({ device, instances, onCancel, backgroundRoute }: Dev
handleRedirection();
};

// @TODO add logic for creating standard/passphrase button
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is forgotten or some unfinished work.

I am not really fan of TODOs in the code. I think you should:

a) finish it
b) or create issue for it and plan it
c) make peace with the fact this will never be done and just ignore it

Without the above, I have an experience that TODOs became just a wishes of the people long gone, polluting the code.

peter-sanderson
peter-sanderson approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@peter-sanderson peter-sanderson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

GOOD JOB!

Most of the comments are nits, but please fix at least the | undefined return and delete the commented code in the storybook.

Then lets 🚢 🇮🇹

@jvaclavik jvaclavik force-pushed the view-only-passphrase-modal branch 2 times, most recently from 82bcf1d to e9ef67e Compare May 15, 2024 07:39
@jvaclavik jvaclavik merged commit 5acdc1b into develop May 15, 2024
22 of 23 checks passed
@jvaclavik jvaclavik deleted the view-only-passphrase-modal branch May 15, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peter-sanderson peter-sanderson peter-sanderson approved these changes

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka MiroslavProchazka is a code owner

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Suite Pull Requests
Status: 🗒 Draft
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jvaclavik @peter-sanderson