Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Provider logic + Controllers #2

Merged
merged 30 commits into from
Apr 12, 2023
Merged

Provider logic + Controllers #2

merged 30 commits into from
Apr 12, 2023

Conversation

ignaciosantise
Copy link
Collaborator

@ignaciosantise ignaciosantise commented Apr 3, 2023

Summary

  • Added provider initialization
  • Added controllers based on web3modal implementation
  • Added basic view to show user address + disconnect button

Screenshots

iOS

RPReplay_Final1680550038.MP4

Android

android.mp4

Copy link

@xzilja xzilja left a comment

Choose a reason for hiding this comment

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

Really cool stuff!

  • I left few comments on code style, that apply all around. I think it might be time to introduce tools like eslint / prettier to catch them? Also perhaps set up github actions to run these checks on pr's (refference in web repo).

  • I noticed that old explorer api is in use, we now have web3modal specific endpoints, feel free to ping me about these to discuss.

  • We can probably remove x1 images (maybe even x2), as likelihood of someone using these even on older phones is very very low and we might benefit from less maintenance, but will leave this up to you.

  • I usually don't leave any comments / todo's in, but since we are early in dev, it's probably ok, just make sure to not forget about them.

@ignaciosantise
Copy link
Collaborator Author

Thanks for your feedback @0xasimetriq 🎉

About image sizes, im following react-native docs recommendations, and i think there's no maintenance problem because Figma has the option to export in all sizes.

@ignaciosantise ignaciosantise requested a review from xzilja April 4, 2023 19:42
@crypblizz8
Copy link
Contributor

I left few comments on code style, that apply all around. I think it might be time to introduce tools like eslint / prettier to catch them? Also perhaps set up github actions to run these checks on pr's (refference in web repo).

+1 @0xasimetriq so let's add some GHub Actions for this. Maybe also let's consider leaving an Expo Snack as our version of Vercel. Then we can easily scan without having to do the xCode / RN CLI setup each time? Do you know of any other streamlined flows @ignaciosantise ?

@ignaciosantise
Copy link
Collaborator Author

@crypblizz8 the repo has some actions, but they execute when the base of the PR is main.

Screen Shot 2023-04-05 at 09 54 49

I'll try to add the QR Code for expo 🎉 good idea

@ignaciosantise ignaciosantise mentioned this pull request Apr 5, 2023
Base automatically changed from initial-sdk-logic to main April 5, 2023 14:04
@crypblizz8
Copy link
Contributor

NonBlocking: Let's also add a README to the main directory and for examples.

@@ -1,4 +1,4 @@
import React, { useMemo } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

I sometimes found that it was necessary during RN Development to keep the React part but not sure if it errors. If no errors, looks good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since React 17, there's no need to import React. I disabled the rule that marked the missing import as an error.

@crypblizz8
Copy link
Contributor

NonBlocking: when I did a yarn on the examples repo, it asked me to
✔ It looks like you're trying to use TypeScript but don't have the required dependencies installed. Would you like to install typescript@^4.9.4, @types/react@~18.0.27? … yes

Is this something we need to add or is it more an expo thing?

@crypblizz8
Copy link
Contributor

IMG_8664
Running into this while doing examples. I did yarn and normal Expo scan/ login. Added projectID. Anything I am missing?

import { ModalCtrl } from '../controllers/ModalCtrl';
import { ClientCtrl } from '../controllers/ClientCtrl';

export function useWeb3Modal() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this export + from initial search in VSCode it shows it is exported in index.tsx. Is there specific usage of this used in examples or elsewhere? Apologies if I am missing something

Copy link
Collaborator Author

@ignaciosantise ignaciosantise Apr 10, 2023

Choose a reason for hiding this comment

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

the exports in index.tsx are the "public" exports from the SDK, that are used in the example app.

in the app we can import this:
import { Web3Modal, Web3Button, useWeb3Modal } from '@web3modal/react-native';

@ignaciosantise
Copy link
Collaborator Author

NonBlocking: when I did a yarn on the examples repo, it asked me to ✔ It looks like you're trying to use TypeScript but don't have the required dependencies installed. Would you like to install typescript@^4.9.4, @types/react@~18.0.27? … yes

Is this something we need to add or is it more an expo thing?

To run this example go to the root folder and:

  1. Run yarn to install all deps
  2. Run yarn example android or yarn example ios

Copy link

@xzilja xzilja left a comment

Choose a reason for hiding this comment

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

Looks good, left few comments 👍

});

// Subscribe to session ping
provider.on(
Copy link

Choose a reason for hiding this comment

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

Should these be unsubscribed from somewhere i.e. instead of useCallback, perhaps put them in useEffect and remove when unmounting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are right, i didn't know there was a function to remove those listeners. Should we add something in the docs?

@ignaciosantise ignaciosantise merged commit 3f6b4ec into main Apr 12, 2023
@ignaciosantise ignaciosantise deleted the provider branch April 12, 2023 16:01
ignaciosantise pushed a commit that referenced this pull request Sep 17, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants