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: add redirectUrl as a constructor option #371

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

SayakMukhopadhyay
Copy link
Contributor

Resolves #367


Behavior

Before the change?

  • The redirect url was only able to be set as a login url parameter

After the change?

  • The redirect url now can also be set as a constructor option.
  • If both are set, the login url parameter is taken precedence

Other information

  • None

Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Feature/model/API additions: Type: Feature

@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Dec 6, 2022
@gr2m gr2m added Type: Feature New feature or request and removed Type: Feature New feature or request labels Dec 6, 2022
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

A+ PR, would merge again :D

Bonus points if you a separate commit with only the tests first next time, so that we see the ❌, then push the fix/feat commits to get the ✅ . That way we can see right away that the tests actually failed before your implementation

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

ahhh forgot one thing: could you update the Constructor options in the README please?

@SayakMukhopadhyay SayakMukhopadhyay force-pushed the redirect-url-options branch 2 times, most recently from aa137d6 to da11a12 Compare December 6, 2022 17:51
@SayakMukhopadhyay
Copy link
Contributor Author

There seems to be conflicts, so I will rebase first.

@SayakMukhopadhyay
Copy link
Contributor Author

Documented the parameter in the README. Also, since I was rebasing to fix the conflict anyway, I went ahead and split the first commit into a test only commit and feature commit.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thank you, really great work 💐💝

@gr2m gr2m merged commit adb57e6 into octokit:master Dec 6, 2022
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SayakMukhopadhyay
Copy link
Contributor Author

Thank you for looking into the PRs so quickly.

@github-actions
Copy link

🎉 This PR is included in version 5.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

[FEAT]: add redirectUrl to constructor option
3 participants