Skip to content

Add twitch socialite auth provider - #684

Merged
ssddanbrown merged 5 commits into
BookStackApp:masterfrom
moutonnoireu:master
Feb 11, 2018
Merged

ssddanbrown merged 5 commits into
BookStackApp:masterfrom
moutonnoireu:master

Conversation

@moutonnoireu

Copy link
Copy Markdown
Contributor

Followed some of the recent socialite added providers (okta, etc). It should work without any problems. If somebody could test ? 👍

@moutonnoireu

Copy link
Copy Markdown
Contributor Author

Forget to run a composer update to update composer.lock.

@moutonnoireu moutonnoireu mentioned this pull request Jan 30, 2018
@lommes

lommes commented Feb 1, 2018

Copy link
Copy Markdown
Member

Thanks for your work!

I just tested this PR and it worked for me! Only thing that would be nice is to change the keys in .env and config/services.php to match those of other services.

TWITCH_KEY should become TWITCH_APP_ID
TWITCH_SECRET should become TWITCH_APP_SECRET

The TWITCH_REDIRECT is not needed in .env since it will be the same for each install of bookstack. It can bet set in config/services.php:

'twitch' => [
        'client_id' => env('TWITCH_APP_ID'),
        'client_secret' => env('TWITCH_APP_SECRET'),
        'redirect' => env('APP_URL') . '/login/service/twitch/callback',
        'name'          => 'Twitch',
    ],

@moutonnoireu

Copy link
Copy Markdown
Contributor Author

Made modifications following your inputs @lommes ;)

@lommes

lommes commented Feb 1, 2018

Copy link
Copy Markdown
Member

There seem to be an issue with the svg file, which is displayed way too large.

@moutonnoireu

Copy link
Copy Markdown
Contributor Author

I reduced the icon size. Was thinking the svg was used dynamically :)

@lommes lommes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had to set width and height attribute of the svg-tag to 24 to make the icon match the button

@lommes

lommes commented Feb 1, 2018

Copy link
Copy Markdown
Member

In my opinion this can be merged if the icon thing is changed.

@moutonnoireu moutonnoireu left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok !

@ssddanbrown

Copy link
Copy Markdown
Member

Thanks both for your efforts on this! I've marked this to be in the next release.

Just one thing I noticed looking over the code, The SVG icon appears to simply be a png embedded within an SVG. It would be great if this was an actual SVG drawing.

I'd be happy to make that change before merging but I'll leave this open until then in case you want to make this change yourself.

@moutonnoireu

Copy link
Copy Markdown
Contributor Author

I'de be happy if you could do it, i'm not very familiar with svg. :)

@ssddanbrown
ssddanbrown merged commit addfb96 into BookStackApp:master Feb 11, 2018
@ssddanbrown

Copy link
Copy Markdown
Member

SVG updated and now merged in, Thanks, Works great!

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

Development

Successfully merging this pull request may close these issues.

3 participants