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(mobile): native connect popup POC #14262

Merged
merged 7 commits into from
Sep 26, 2024
Merged

feat(mobile): native connect popup POC #14262

merged 7 commits into from
Sep 26, 2024

Conversation

Nodonisko
Copy link
Contributor

@Nodonisko Nodonisko commented Sep 10, 2024 •
edited by martykan
Loading

Description

Don't forget to run yarn prebuild if you do any changes to app.config.ts

Related Issue

Resolve

Screenshots:

Screen.Recording.2024-09-24.at.14.09.50.mov

@vytick vytick added the mobile Suite Lite issues and PRs label Sep 10, 2024
@martykan martykan force-pushed the feat/native-connect-popup branch 4 times, most recently from a2c79b0 to 68b50f7 Compare September 20, 2024 09:59
@martykan martykan changed the title feat(mobile): WIP native connect popup feat(mobile): native connect popup POC Sep 20, 2024
martykan
martykan reviewed Sep 20, 2024
View reviewed changes
suite-native/module-connect-popup/src/screens/ConnectPopupScreen.tsx Outdated
}

if (discoveryActive) {
return <Loader size="large" title="Discovery running, pls wait :(" />;
Copy link
Member

Choose a reason for hiding this comment

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

let's follow up on this later

@karliatto
Copy link
Member

@Nodonisko We think this PR would be useful to be in develop branch so we can continue developing to improve things in further PRs.
And it would be great if you could review this PR at least the suite-native section. 🙏

@karliatto karliatto marked this pull request as ready for review September 23, 2024 07:33
@karliatto karliatto requested review from a team, mroz22 and szymonlesisz as code owners September 23, 2024 07:33
@martykan
Copy link
Member

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/11012717275

@trezor-ci trezor-ci force-pushed the feat/native-connect-popup branch from 5572148 to 47deadd Compare September 24, 2024 11:22
@Nodonisko
Copy link
Contributor Author

Good job, overall it looks really nice!

@martykan martykan force-pushed the feat/native-connect-popup branch from 47deadd to 5d301f8 Compare September 24, 2024 11:58
@Nodonisko Nodonisko requested a review from matejkriz September 24, 2024 11:58
@Nodonisko
Copy link
Contributor Author

Can you please maybe add some video how does it looks like now?

@martykan martykan force-pushed the feat/native-connect-popup branch 2 times, most recently from 0441279 to afb76de Compare September 24, 2024 13:21
@vytick
Copy link
Contributor

vytick commented Sep 24, 2024 •
edited
Loading

Very good job guys :)

Please, what is the best way to test it? How can I play with it?

@martykan
Copy link
Member

Best way to test it the whole process is to run suite-native/connect-deeplink-example
It showcases just 1 method call but it's easy to modify

matejkriz
matejkriz reviewed Sep 25, 2024
View reviewed changes
suite-native/module-connect-popup/src/components/ButtonRequestsOverlay.tsx
const getButtonRequestComponent = (request: ButtonRequest) => {
if (request.code === 'ButtonRequest_Address') {
return (
<Box paddingBottom="extraLarge" key={request.code}>
Copy link
Member

Choose a reason for hiding this comment

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

nit: request.code may not be unique, there can be multiple button request with the same code on the device...

matejkriz
matejkriz reviewed Sep 25, 2024
View reviewed changes
suite-native/module-authorize-device/src/components/passphrase/PassphraseForm.tsx
@@ -150,6 +156,7 @@ export const PassphraseForm = ({ inputLabel, onFocus }: PassphraseFormProps) =>
horizontalMargin={FORM_CARD_PADDING}
/>
<EnterPassphraseOnTrezorButton />
{noPassphraseEnabled && <NoPassphraseButton />}
Copy link
Member

Choose a reason for hiding this comment

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

It's just a workaround, right? I guess in the end we should not request passphrase for standard wallets and user will have to choose the wallet before processing the request. Another workaround for all devices except T1 is entering passphrase on Trezor (you can enter empty passphrase there)...

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind this workaround, it does not affect the regular flow, so we can merge it.

@Nodonisko
Copy link
Contributor Author

lgtm

@martykan martykan force-pushed the feat/native-connect-popup branch 2 times, most recently from 5dea4aa to ef51c96 Compare September 25, 2024 13:56
@martykan martykan force-pushed the feat/native-connect-popup branch from 55ee24f to c7f9d1f Compare September 26, 2024 12:05
@martykan martykan merged commit c727023 into develop Sep 26, 2024
88 checks passed
@martykan martykan deleted the feat/native-connect-popup branch September 26, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vytick vytick vytick left review comments

@martykan martykan martykan left review comments

@matejkriz matejkriz matejkriz approved these changes

@karliatto karliatto Awaiting requested review from karliatto

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

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

Assignees
No one assigned
Labels
mobile Suite Lite issues and PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nodonisko @karliatto @martykan @vytick @matejkriz