OKO-522 #183
OKO-522 #183
Conversation
| } from "@oko-wallet-sdk-core/ui/login_modal"; | ||
|
|
||
| // Prevent multiple modals from being opened simultaneously | ||
| let activeModalController: LoginModalController | null = null; |
There was a problem hiding this comment.
I'm against the naming. It gives me the impression that this is a class instance.
There was a problem hiding this comment.
Also, could you "const"-ify this variable then have the code mutate the property?
e.g., some_state = { mutable: null }
|
I merged main into this PR for CI type-check. If you don't want it, you can revert that. |
| // Not signed in - trigger OAuth sign in | ||
| await okoSolWallet.okoWallet.signIn("google"); | ||
| // Not signed in - open provider select modal | ||
| await okoSolWallet.okoWallet.login(); |
There was a problem hiding this comment.
openLogInModal() 같은 느낌의 이름으로 해야할듯 합니다
There was a problem hiding this comment.
Renamed it to openSignInModal() since it essentially wraps the existing signIn method
| export function renderLoginModal( | ||
| options: LoginModalOptions, | ||
| ): LoginModalController { | ||
| const { onSelect, onClose, theme = "system" } = options; |
There was a problem hiding this comment.
Why don't you use React? This will very rapidly become hard to maintain
There was a problem hiding this comment.
I think React might not be the best fit since this is a framework-agnostic SDK that needs to work in any app (React, Vue, Angular, vanilla JS). Adding React could force dependencies on consumers, cause version conflicts, and increase bundle size for a simple modal.
There was a problem hiding this comment.
I do agree maintainability is important, so I refactored to use HTML templates. What do you think of this approach?
| @@ -0,0 +1,364 @@ | |||
| export const MODAL_STYLES = ` | |||
There was a problem hiding this comment.
If you decide to use some ui drawing lib like React, the style definition will also need to be adjusted. (in a safer way)
|
|
||
| contentWrapper.addEventListener("click", (e) => { | ||
| const btn = (e.target as HTMLElement).closest("button") as HTMLButtonElement | null; | ||
| if (!btn || btn.disabled) return; |
| const resolvedTheme = resolveTheme(theme); | ||
|
|
||
| const container = document.createElement("div"); | ||
| container.id = "oko-signin-modal"; |
There was a problem hiding this comment.
please const-ify this string
| return getHostTheme() ?? getSystemTheme(); | ||
| } | ||
|
|
||
| function defaultViewTemplate(theme: ResolvedTheme): string { |
| "@oko-wallet/oko-types": "^0.0.1-alpha.5", | ||
| "@oko-wallet/stdlib-js": "^0.0.2-rc.45" | ||
| "@oko-wallet/stdlib-js": "^0.0.2-rc.45", | ||
| "preact": "^10.25.4" |
Pull Request
CONTRIBUTING.mdand followed the guidelines.Summary
oko_sdk_core: add login modal for OAuth provider selection
Links (Issue References, etc, if there's any)