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

[default-login] Allow overriding provider list #461

Merged
merged 1 commit into from Jan 10, 2018
Merged

[default-login] Allow overriding provider list #461

merged 1 commit into from Jan 10, 2018

Conversation

skogsmaskin
Copy link
Member

Allow overriding and configuring the provider list in the default-login part.

This is to support the third party login enterprise features.

@skogsmaskin skogsmaskin force-pushed the feature/default-login-custom-providers branch 2 times, most recently from b13eec4 to 19d64e9 Compare January 8, 2018 14:03
bjoerge
bjoerge reviewed Jan 8, 2018
View reviewed changes
Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

A few comments and questions, otherwise LGTM.

packages/@sanity/default-login/README.md Outdated
```
{
// Your custom provider implementations
"customProviders": [
Copy link
Member

Choose a reason for hiding this comment

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

For the sake of brevity, could this key be just providers?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my first name, however it was pretty confusing, because they can be appended to or replace the existing providers. I think this made it more clear what this really is.

packages/@sanity/default-login/README.md Outdated
@@ -19,14 +54,14 @@ static propTypes = {
]).isRequired,
title: PropTypes.string,
description: PropTypes.string,
sanityLogo: PropTypes.node
SanityLogo: PropTypes.component
Copy link
Member

Choose a reason for hiding this comment

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

Don't think PropTypes.component exists? At least it's undocumented: https://reactjs.org/docs/typechecking-with-proptypes.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Doh, invalid readme. The code is ok though.

packages/@sanity/default-login/config.dist.json
@@ -0,0 +1,5 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

What happens if non-enterprise users adds providers here?

Copy link
Member Author

Choose a reason for hiding this comment

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

They will be sent to whatever URL they give, however they will not be able to produce an actual Sanity session on their side.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should warn or give some kind of feedback if that happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we now warn when the custom provider logo is clicked and the project is missing the feature. In this way someone can implement this before they get the actual bit.

kristofferjs
kristofferjs approved these changes Jan 8, 2018
View reviewed changes
Copy link
Contributor

@kristofferjs kristofferjs left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe the default svg logo could be placed in the base/components/icons if we want to reuse it (base part).

@bjoerge
Copy link
Member

bjoerge commented Jan 9, 2018

👍 to @kristofferj's suggestion about the question mark icon

@skogsmaskin skogsmaskin force-pushed the feature/default-login-custom-providers branch 3 times, most recently from e4e5830 to e672c72 Compare January 9, 2018 13:40
@bjoerge bjoerge force-pushed the next branch 2 times, most recently from 4bc05d2 to e963d9b Compare January 9, 2018 15:59
@skogsmaskin skogsmaskin force-pushed the feature/default-login-custom-providers branch from e672c72 to 889af3f Compare January 10, 2018 10:21
@skogsmaskin skogsmaskin merged commit 7a3ef8c into next Jan 10, 2018
@skogsmaskin skogsmaskin deleted the feature/default-login-custom-providers branch January 10, 2018 10:24
bjoerge pushed a commit that referenced this pull request Jan 10, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
2b3b412 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
rexxars pushed a commit that referenced this pull request Jan 10, 2018
@skogsmaskin @rexxars
[default-login] Allow overriding provider list through json config fi…
5284b4d 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
kristofferjs pushed a commit that referenced this pull request Jan 11, 2018
@skogsmaskin @kristofferjs
[default-login] Allow overriding provider list through json config fi…
f4bcb3e 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 16, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
3943be2 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 17, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
d7109c0 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 19, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
dcf8fcf 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 24, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
14bf41e 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 25, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
b8a6fc7 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 28, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
4db4dc9 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 28, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
b676553 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
bjoerge pushed a commit that referenced this pull request Jan 30, 2018
@skogsmaskin @bjoerge
[default-login] Allow overriding provider list through json config fi…
e2811e8 
…le (#461)

Allow overriding and configuring the provider list in the default-login part.
This is to support the third party login enterprise features.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kristofferjs kristofferjs kristofferjs approved these changes

@bjoerge bjoerge bjoerge approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@skogsmaskin @bjoerge @kristofferjs