-
Notifications
You must be signed in to change notification settings - Fork 268
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: adds advancedOption to getWithPopup #1566
base: 7.10
Are you sure you want to change the base?
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.
Should we document monitorPopupWindow
in readme?
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 looks good, but I suggested a few grammatical updates.
I also feel that we should document this in some way, even if just to take the code comment as docs (it can be short & sweet, but probably important to explain that it's an advanced use-case).
// WARNING: This resulting promise will remain pending until the user completes the flow or timeout | ||
if (options?.monitorPopupWindow === false) { | ||
if (!options.timeout) { | ||
throw new AuthSdkError('`timeout` must be set when `monitorPopupWindow` is set to `false`!'); |
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.
Very nice touch!
// `monitorPopupWindow: false` is an advance setting. Use with caution! | ||
// When `false`, polling to determine whether or not the popup is open is perforemd. | ||
// A `timeout` must be provided when `monitorPopupWindow` is set to `false` |
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.
// `monitorPopupWindow: false` is an advance setting. Use with caution! | |
// When `false`, polling to determine whether or not the popup is open is perforemd. | |
// A `timeout` must be provided when `monitorPopupWindow` is set to `false` | |
// `monitorPopupWindow: false` is an advanced setting. Use with caution! | |
// When `false`, polling to determine whether or not the popup is open will not be performed. | |
// If using this option to disable polling, a `timeout` value must also be provided. |
|
||
### Other | ||
|
||
- [#1566](https://github.com/okta/okta-auth-js/pull/1566) feat: adds advance option `monitorPopupWindow` to `getWithPopup` |
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.
- [#1566](https://github.com/okta/okta-auth-js/pull/1566) feat: adds advance option `monitorPopupWindow` to `getWithPopup` | |
- [#1566](https://github.com/okta/okta-auth-js/pull/1566) feat: adds the advanced option `monitorPopupWindow` to `getWithPopup`. |
No description provided.