-
Notifications
You must be signed in to change notification settings - Fork 137
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
feat: add useExportAccount hook #567
Conversation
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.
same comment here as in the other ones -- docs :)
e5ad797
to
7144c68
Compare
7144c68
to
00c9eb0
Compare
36923ee
to
13b8ceb
Compare
00c9eb0
to
58597f8
Compare
0327d40
to
f1c986b
Compare
58597f8
to
4ab6722
Compare
f1c986b
to
b7a9bc0
Compare
4ab6722
to
7cb782b
Compare
b7a9bc0
to
e3de338
Compare
33413f4
to
57dacd7
Compare
e3de338
to
c14022c
Compare
params?: ExportAccountParams; | ||
} & BaseHookMutationArgs<UseExportAccountData, UseExportAccountParams>; | ||
|
||
export type ExportAccountComponentProps = { |
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.
How or why this should be used is not clear, we should document this and put some comments around 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.
this is documented on the account kit docs and in the example snippet, but agree, also put js docs on this
style: { display: !isExported ? "none" : "block" }, | ||
id: iframeContainerId, | ||
}, | ||
createElement("style", {}, iframeCss) |
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.
Why do we want to create a <style>
element inside of the iframe container div?
What is the expected escape hatch you are trying to do here? I'm a little confused by that. Can we add what this does in documentation somewhere? (Hence the push for jsdocs)
Also if you are trying to add style to the iframe itself, why not just add it in line 68?
Side note you can always make iframeCss
be a React.CSSProperties
and then just merge object that with whatever existing styles you want? It'll probably make downstream code more type safe.
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 why is mainly because this is what turnkey's docs show for styling the iframe. Maybe it should be just css props and the style component always is like
iframe { ${iframeCSS} }
Or h/e that works
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.
it's a little different even doing iframe { ${iframeCSS} }
because if iframeCSS is React.CSSProperties
then it's still not a css string: it's an object which we can't just stringify (keys need to go from camel case to kebab case, and we need to separate key-vals with ";")
b/c it's more typesafe, i've added a parser to continue doing the iframe styling as turnkey recommends @moldy530 @rthomare. lmk what you think.
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.
just commenting to flag
isExported: boolean; | ||
}; | ||
|
||
export type UseExportAccountResult = { |
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 code here in this hook assumes web environment.
Can we make it generic so that the client can also be non-web react environment but also react native? (i.e. webview)
The API between webview <> iframe should be similar/compatible, but haven't looked deeply myself yet if having a shared react hook for this hook between web vs native env would work/be better.
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.
probably something we'd fast follow with, separating this as a devex improvement vs. net-new functionality for react native.
57dacd7
to
fb55de7
Compare
85f5b5b
to
a2f5510
Compare
2437cc0
to
4b54aee
Compare
Pull Request Checklist
yarn test
)site
folder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:write
)PR-Codex overview
This PR introduces a new
useExportAccount
hook for exporting account recovery details in Alchemy.Detailed summary
useExportAccount
hook with related types and componentsuseExportAccount
hook in Markdown format