Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Update element to use signin-aware #6

Merged
merged 4 commits into from
Jul 7, 2014
Merged

Update element to use signin-aware #6

merged 4 commits into from
Jul 7, 2014

Conversation

Scarygami
Copy link
Contributor

As discussed with @addyosmani at GoogleWebComponents/google-signin#11

My suggestion how to change/use the google-calendar element to allow for one signin button on the page and no authentication interference between multiple elements.

@@ -96,7 +95,7 @@
*/
displayCalendars: function() {
if (this.signedIn && this.$.calendar.api) {
var request = this.$.calendar.api.calendarList.list();
var request = this.$.calendar.api.calendarList.list({"key": ""});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reasoning for the {"key": ""} parameter:

On the demo page another element is included that only uses an apiKey by setting gapi.client.setApiKey

In some cases (not consistently) some Google API methods return a 401 invalid credentials error when both an Authorization header and a key=xxx query parameter are provided (there's an issue open for that somewhere for quite a while already...), and I actually ran into this issue while testing.

To prevent this I explicitly set the key query parameter to be empty here, overwriting the default that would come from gapi.auth.setApiKey

@ebidel
Copy link
Contributor

ebidel commented Jun 17, 2014

@devnook can you take a look at this?

@addyosmani
Copy link
Member

@Scarygami This one will also need to be rebased before it can be merged in if we go ahead with it.

@Scarygami
Copy link
Contributor Author

Rebase done 👍

@addyosmani
Copy link
Member

Thank you!

@addyosmani
Copy link
Member

ping @devnook

@addyosmani
Copy link
Member

Merging in as changes LGTM and there's value in getting this in before PRs build up.

cc @devnook in case there's anything you think needs further work or requires a revert.

addyosmani added a commit that referenced this pull request Jul 7, 2014
Update element to use signin-aware
@addyosmani addyosmani merged commit e9fe3ba into GoogleWebComponents:master Jul 7, 2014
@Scarygami
Copy link
Contributor Author

@addyosmani would it make sense if I send PRs to other elements using the signin element to update them to signin-aware as well, or will you discuss/update this internally first?

@addyosmani
Copy link
Member

Yes please :) I think it makes sense to update the other elements to use signin-aware and I'm happy to help cc in the right folks as an fyi who would be useful to sanity check on reviews.

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

Successfully merging this pull request may close these issues.

None yet

3 participants