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.

Sign up for GitHub

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

Jump to bottom

Provide dedicated page per authentication provider #867

Merged
merged 6 commits into from
Aug 15, 2022
Merged

Provide dedicated page per authentication provider #867

merged 6 commits into from
Aug 15, 2022

Conversation

cynthia-sg
Copy link
Contributor

This PR splits the Authentication Providers documentation page content into
multiple pages. A dedicated page per authentication provider has been added,
and the main page now includes a list of the available providers with a link to
each page. This change has been applied to versions 2.7 and 2.8.

Fixes #711

Signed-off-by: Cintia Sanchez Garcia [email protected]

@cynthia-sg
Provide dedicated page per authentication provider
5b2a859 
This PR splits the `Authentication Providers` documentation page content into
multiple pages. A dedicated page per authentication provider has been added,
and the main page now includes a list of the available providers with a link to
each page. This change has been applied to versions 2.7 and 2.8.

Fixes kedacore#711

Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@netlify
Copy link

netlify bot commented Aug 4, 2022 •
edited
Loading

✅ Deploy Preview for keda ready!

Name Link
🔨 Latest commit e8eba85
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/62f9dd8545a44e000811a2b6
😎 Deploy Preview https://deploy-preview-867--keda.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cynthia-sg cynthia-sg mentioned this pull request Aug 4, 2022
Closed
tomkerkhove
tomkerkhove reviewed Aug 4, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Did a first skim and I love it!

content/docs/2.7/providers/azure-workload.md Outdated Show resolved Hide resolved
content/docs/2.7/concepts/authentication.md Show resolved Hide resolved
layouts/partials/nav.html Outdated Show resolved Hide resolved
chalin
chalin reviewed Aug 4, 2022
View reviewed changes
layouts/partials/nav.html Outdated Show resolved Hide resolved
chalin
chalin requested changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Very nice work @cynthia-sg, thanks! See my inline comments.

@chalin
Copy link
Contributor

chalin commented Aug 4, 2022

@tomkerkhove - This PR presents the Authentication Providers in the left nav as a drop down. Do you expect this list of providers to grow like for that for Scalers? If so, consider not listing the providers via a drop down.

@chalin
Copy link
Contributor

chalin commented Aug 4, 2022

@cynthia-sg - I have some of the changes that I suggested via the review already done in my local repo, let me know if you'd like for me to push those up.

@tomkerkhove
Copy link
Member

@tomkerkhove - This PR presents the Authentication Providers in the left nav as a drop down. Do you expect this list of providers to grow like for that for Scalers? If so, consider not listing the providers via a drop down.

I don't think we'll have more than 10 or so long term but it does not have to be a dropdown perse; I would just like to have a shortcut here:
image

tomkerkhove
tomkerkhove reviewed Aug 4, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Would you mind adding a quick section in the readme on how to add a new provider similar to what we have for scalers please?

@cynthia-sg
Address feedback
f00f8dd 
Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@JorTurFer
Copy link
Member

awesome work! One thing, maybe we should add a link or a note in the Authentication section pointing to this because they are going to use there

@cynthia-sg
Copy link
Contributor Author

@cynthia-sg - I have some of the changes that I suggested via the review already done in my local repo, let me know if you'd like for me to push those up.

I’ve already addressed them, but thanks anyway!

@cynthia-sg
Copy link
Contributor Author

@tomkerkhove - This PR presents the Authentication Providers in the left nav as a drop down. Do you expect this list of providers to grow like for that for Scalers? If so, consider not listing the providers via a drop down.

I don't think we'll have more than 10 or so long term but it does not have to be a dropdown perse; I would just like to have a shortcut here: image

The link for the Authentication Providers is there in fact. I added it for mobile version but it's hidden for desktop using the class providers-link. I agree with @chalin about the navbar is crowded and perhaps it's necessary to change this.

@cynthia-sg
Copy link
Contributor Author

Would you mind adding a quick section in the readme on how to add a new provider similar to what we have for scalers please?

Added! Please feel free to update the template for the provider.

@cynthia-sg
Add outlined class to providers buttons
a30ba0c 
Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@cynthia-sg
Copy link
Contributor Author

awesome work! One thing, maybe we should add a link or a note in the Authentication section pointing to this because they are going to use there

Thanks! I'm not sure what you mean. Right now providers are listed at the end of the Authentication section. Do you want a link/note in another part of the section too?

@JorTurFer
Copy link
Member

awesome work! One thing, maybe we should add a link or a note in the Authentication section pointing to this because they are going to use there

Thanks! I'm not sure what you mean. Right now providers are listed at the end of the Authentication section. Do you want a link/note in another part of the section too?

oh, that's true...
Ignore me

chalin
chalin approved these changes Aug 6, 2022
View reviewed changes
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline for comments about terminology, which might affect titles. Otherwise, LGTM!

README.md Outdated Show resolved Hide resolved
content/docs/2.7/concepts/authentication.md Show resolved Hide resolved
content/docs/2.7/providers/azure-key-vault.md Outdated Show resolved Hide resolved
layouts/partials/content.html Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

@tomkerkhove - This PR presents the Authentication Providers in the left nav as a drop down. Do you expect this list of providers to grow like for that for Scalers? If so, consider not listing the providers via a drop down.

I don't think we'll have more than 10 or so long term but it does not have to be a dropdown perse; I would just like to have a shortcut here: image

The link for the Authentication Providers is there in fact. I added it for mobile version but it's hidden for desktop using the class providers-link. I agree with @chalin about the navbar is crowded and perhaps it's necessary to change this.

Fine by me, but then it should be in this drop-down at least because it's hidden atm:
image

tomkerkhove
tomkerkhove approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a ton for this!

Here are my last remarks along with some comments above; for the rest things should be OK!

content/docs/2.7/concepts/authentication.md Outdated Show resolved Hide resolved
content/docs/2.7/concepts/authentication.md Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Sorry for all the comments but I was wondering if you are up for updating this PR @cynthia-sg? I appreciate the big help here, this is tremendous help!

@cynthia-sg
Copy link
Contributor Author

Sorry for all the comments but I was wondering if you are up for updating this PR @cynthia-sg? I appreciate the big help here, this is tremendous help!

Hi @tomkerkhove,

Yes, I will take care of it soon, leave it with me. I’ve been a few days off on vacation :)

@tomkerkhove
Copy link
Member

No worries at all and hope you've enjoyed your time off!

@cynthia-sg
Address feedback
1caeb73 
Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@cynthia-sg
Revert blank line deleted by mistake
c6368a4 
Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@tomkerkhove
Copy link
Member

tomkerkhove commented Aug 13, 2022 •
edited
Loading

Can we add a link on the landing page as well?

Screenshot_20220813-193827.png

tomkerkhove
tomkerkhove approved these changes Aug 13, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Added one comment about landing page but love it, thanks a ton!

@tomkerkhove
Copy link
Member

Still not sold on the side nav missing a link per provider though be we can continue that in a seperate PR/issue 🙂

@cynthia-sg
Add like to landing page
e8eba85 
Signed-off-by: Cintia Sanchez Garcia <[email protected]>
@cynthia-sg
Copy link
Contributor Author

Can we add a link on the landing page as well?

Screenshot_20220813-193827.png

Added! 👍
keda-home

@tomkerkhove
Copy link
Member

Thanks a ton!

@tomkerkhove tomkerkhove merged commit f7557f7 into kedacore:main Aug 15, 2022
@tomkerkhove
Copy link
Member

I love it.

@cynthia-sg
Copy link
Contributor Author

I love it.

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomkerkhove tomkerkhove tomkerkhove approved these changes

@chalin chalin chalin approved these changes

@jeffhollan jeffhollan Awaiting requested review from jeffhollan

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

@zroubalik zroubalik Awaiting requested review from zroubalik

@JorTurFer JorTurFer Awaiting requested review from JorTurFer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide dedicated page per authentication provider
4 participants
@cynthia-sg @chalin @tomkerkhove @JorTurFer