Skip to content
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

Open
wants to merge 2 commits into
base: 7.10
Choose a base branch
from

Conversation

jaredperreault-okta
Copy link
Contributor

No description provided.

Copy link
Contributor

@denysoblohin-okta denysoblohin-okta left a 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?

Copy link

@mikenachbaur-okta mikenachbaur-okta left a 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`!');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice touch!

Comment on lines +20 to +22
// `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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// `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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [#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`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants