Skip to content

Conversation

@vfiruz97
Copy link

Copy link
Collaborator

@marcelomendoncasoares marcelomendoncasoares left a comment

Choose a reason for hiding this comment

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

Thanks for the quick iteration with the docs, @vfiruz97! Docs looks good overall, following the standard of the other IDPs very well. Made a few suggestions to clarify some points and reorganize some details to improve the UX.

On the custom overrides, I think we'll need to convert it into a menu following the same style of the IDPs - after all, it is just like an IDP. The file is currently very long and with parts that can be removed to avoid redundancy.

Ping me back when you are ready for another review!

@vfiruz97
Copy link
Author

Hi @marcelomendoncasoares,
Great review, thanks a lot. I have addressed them and everything's ready for you to check.

Copy link
Collaborator

@marcelomendoncasoares marcelomendoncasoares left a comment

Choose a reason for hiding this comment

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

Nice improvements! A few more comments, specially regarding the OAuth2 utility setup guide. I have answered about the GitHubOAuthCredentials on the comments.

Also, there are a few links that needs fixing after transforming the custom providers into a menu.

  Exhaustive list of all broken links found:
  - Broken link on source page path = /next/concepts/authentication/basics:
     -> linking to providers/custom-providers (resolved as: /next/concepts/authentication/providers/custom-providers)
  - Broken link on source page path = /next/concepts/authentication/providers/custom-providers/oauth2-utility/oauth2-utility-basic:
     -> linking to ../github/setup (resolved as: /next/concepts/authentication/providers/custom-providers/github/setup)
  - Broken link on source page path = /next/concepts/authentication/providers/custom-providers/overview:
     -> linking to ./oauth2-utility (resolved as: /next/concepts/authentication/providers/custom-providers/oauth2-utility)

@vfiruz97
Copy link
Author

Hi @marcelomendoncasoares,
Great suggestions as always! I've addressed all comments. Also fixed broken links and the menu order.
Ready for review!

We removed GitHubOAuthCredentials from the docs, and I'll apply the same fix in serverpod as well.
Would you prefer that I push the fix to the already merged PR (#4546), or open a fresh PR after syncing with main and rebasing vfiruz97:feat/github-idp. Also I would like to implement the discussed OAuth2PkceTokenResponse change from #4642?

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.

2 participants