-
Notifications
You must be signed in to change notification settings - Fork 3
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
Change wallet #109
base: main
Are you sure you want to change the base?
Change wallet #109
Conversation
Deploying delegit with Cloudflare Pages
|
Thank you for the feedback. There are still some edge cases (and some non edge ones) that im still fixing, making it more DX. The most important is that i have now multiple views available that can be dedicated per device. Also most of the design elements can be parameterized by the config so design wise that will be super easy to fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry in advance for the amount of comments, I'm fresh and have many questions.
As a dev, I really just want to get the accounts
, selectAccount
, selectedAccount
. If there's any localstorage, I should take care of this, just like we did with Tien's modal.
Finally I couldn't actually play with it as I got the following error
SyntaxError: The requested module 'http://localhost:5173/node_modules/.vite/deps/@polkadot-ui_react.js?v=a9b536dc' doesn't provide an export named: 'useAccountLocalStorage'
} | ||
|
||
const AccountContext = createContext<IAccountContext | undefined>(undefined) | ||
|
||
const AccountContextProvider = ({ children }: AccountContextProps) => { | ||
const accounts = useRedotAccounts() | ||
const [localStorageAccount, setLocalStorageAccount] = useAccountLocalStorage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a weird DX that I've never seen before. I don't understand why we'd need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the wallet the localstorage has a specific key. This hook is allowing you to get/set that key without the need to know the key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do I really need this though, as a dev? I'm still not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean "as a dev"... how will you setup the localstorage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at Tien's interface for the dot-connect, there's no mention of local storage ever. If we do anything with local storage, it's on our side. This useAccountLocalStorage
comes form polkadot-ui
and I don't get why. I think these things should be hidden if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted!
type="extensions" | ||
config={connectConfig} | ||
selected={selectedAccount} | ||
setSelected={selectAccount} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the naming setSelected
and selectAccount
looks weird here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.. Opened polkadot-ui/library#356 and will fix in next version of the library release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I messed up yesterday and hadn't installed de deps, I did play with it in the end. Functionality looks good, just the implementation feel non intuitive, with things I wouldn't expect us to have to deal with, see my previous comments.
@Tbaut I have no idea why the build and lint are failing here 🤷🏽 |
The NodeJS server is extremely slow and the CI doesn't manage to download it in time, I guess it'll be better when we retry later. I relaunched it, and tried myself, but it's simply super slow, takes 5+min and times out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are stuff I just don't see why they are here. Maybe it's just me, but without any background, it's just confusing and will make the maintenance harder:
- I don't want any local storage handling from the lib. we care about the last selected account 100%, or the lib cares about it 100%. this mix that we have here isn't clear
- same for the
accounts
. I see asetAccounts
passed in theConnectModal
. I don't think this is right. I should just get them e.g withconst {accounts} = useConnect()
and not have to care about the state. we never use it, so it shouldn't be exposed. - similarly, I'm not sure why we need to pass
selected
andsetSelected
accounts. The modal doesn't do anything with them.
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
Modal has different forms ("type") and some of them handle the accounts. But you are right that in the current state (only extensions) it does not. Will adjust accordingly |
closes #107
fixes #76
Submission checklist:
Layout