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

Enable handling users closing login popup #1058

Merged
merged 6 commits into from Apr 25, 2021
Merged

Enable handling users closing login popup #1058

merged 6 commits into from Apr 25, 2021

Conversation

adiessl
Copy link
Contributor

@adiessl adiessl commented Apr 22, 2021

I noticed there is no way of getting notified when a user closes the login popup. The registered event handler in the main window also does not get removed in this case. This PR solves these issues.

Open questions to discuss:

  • Is it problematic if accessToken and userData are undefined in LoginResult?
  • Should an errorMessage property be added to LoginResult?
    {
      isAuthenticated: false,
      errorMessage: "User closed popup"
    }

@FabianGosebrink
Copy link
Collaborator

FabianGosebrink commented Apr 22, 2021

Hey @adiessl, that is a nice fix, implemented in a good and clean way. I like it! Nice work, really well done.

Concerning your questions:

Is it problematic if accessToken and userData are undefined in LoginResult?

Currently the popup login method is the only one returning those properties, IMHO it is not problematic having those to be undefined, becase the user closed.

Should an errorMessage property be added to LoginResult?

IMHO this would be a nice improvement to be clear and transparent what happened. Would you like to add it?

@adiessl
Copy link
Contributor Author

adiessl commented Apr 22, 2021

Sure, I'll add the errorMessage.

Repository owner deleted a comment from inloox-dev Apr 22, 2021
@FabianGosebrink FabianGosebrink self-assigned this Apr 23, 2021
@damienbod damienbod merged commit 5fa422d into damienbod:main Apr 25, 2021
@damienbod
Copy link
Owner

LGTM thanks! will be released in 11.6.7

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.

None yet

3 participants