feat(auth-modal): onSuccess, onDismiss, and onError callbacks #3481
Conversation
miguelpeixe
added
[Status] Needs Review
There was a problem hiding this comment.
Approving this one but just noting I had a non-blocking nit/question about this here: Automattic/newspack-blocks#1908 (comment)
github-actions
bot
added
[Status] Approved
miguelpeixe
changed the title
| if ( config.callback ) { | ||
| config.callback(); | ||
| if ( config.onSuccess && typeof config.onSuccess === 'function' ) { | ||
| config.onSuccess(); |
There was a problem hiding this comment.
Non-blocking suggestion: You can remove the need for these checks throughout the PR (config.onSuccess && ... === 'function') by just calling config?.onSuccess?.()
There was a problem hiding this comment.
This will still throw an error if onSuccess is truthy an not a function.
There was a problem hiding this comment.
Hmm, this bit should cause this to resolve as undefined when not a function: onSuccess?.()
There was a problem hiding this comment.
I double-checked running const notAFn = true; notAFn?.(); in the console and it errored.
There was a problem hiding this comment.
Right. This won't work on every browser, but our build process will be sure to compile this to something universally compatible. This isn't a blocker though so feel free to keep what you have!
There was a problem hiding this comment.
There was a problem hiding this comment.
I'd rather keep it because the optional chaining operator will not do type validation.
src/reader-activation-auth/index.js
Outdated
| window.location.href = redirect; | ||
| }; | ||
| } | ||
|
|
||
| openAuthModal( { | ||
| callback, | ||
| onSuccess, | ||
| onError: onSuccess, |
There was a problem hiding this comment.
Let's rename the callback to something other than onSuccess since its meant to be called even when auth is unsuccessful. callback seemed fine to me.
|
Thanks for reviewing, @chickenn00dle! |
All Submissions:
Changes proposed in this Pull Request:
Adds support for a callback when the auth modal closes without authenticating.
How to test the changes in this Pull Request:
Context and instructions at Automattic/newspack-blocks#1908
Other information: