aspnet-contrib / AspNet.Security.OpenIdConnect.Samples

ASP.NET Core samples demonstrating how to use the OpenID Connect server with MVC or JS apps
64 stars 31 forks source link

Added Cordova sample : #8

Closed pjeannet closed 8 years ago

pjeannet commented 8 years ago

The Backend part is the same as MVC.Server, just changed the uri_redirect in startup.cs to target the app. The MobileApp is built with Cordova and Ionic.

kevinchalet commented 8 years ago

Thanks! :clap:

/cc @DovydasNavickas as a JS expert, do you wanna take a look? :smile:

DovydasNavickas commented 8 years ago

During weekend I can :)

DovydasNavickas commented 8 years ago

Ooook. I'm not Cordova specialist (i.e. never compiled an app with it), but... What is this and this?

Also, I would move configuration (such as this and URL from this) out into a single file, e.g. config.js or sth, because having to change everything throughout the solution is not really helping with clarity.

A minor thing about different naming case here and here

Also this and this should not be in the middle of authService.js :smile:

And a mistype in a libraries folder name here (or is it in French?).

Then there's a question whether to keep the dummy methods here and a dummy files here and here. These files with non-English comments do no good for the sample :)

All in all, the sample looks OK and if it works, then there are only these minor things to fix. Apart from that, I'd say having everything written in English (including comments) is the most beneficial if you want to reach most people.

Thank you for the PR! :clap: :smile:

pjeannet commented 8 years ago

Ooook. I'm not Cordova specialist (i.e. never compiled an app with it), but... What is this and this?

I think it was to check if the user was not already authenticated and to avoid calling login function. However the button for login is hidden if user is authenticated so it can be remove. (TBH I think it was part of some code that I copy/pasted when I tried cordova projects)

Also, I would move configuration (such as this and URL from this) out into a single file, e.g. config.js or sth, because having to change everything throughout the solution is not really helping with clarity.

As it was just a sample, I didn't want to add too much files, but I think we can add it in router.js file that needs a renaming like init.js as the angular app is initialized in it and there is no routing in this app!

A minor thing about different naming case here and here

Will correct it!

Also this and this should not be in the middle of authService.js :smile:

I agree, I was lazy to put it somewhere else as it was a simple sample. Will correct it!

And a mistype in a libraries folder name here (or is it in French?).

Libraries is just the plural for Library, it seems to me that it is valid English ;)

Then there's a question whether to keep the dummy methods here and a dummy files here and here. These files with non-English comments do no good for the sample :)

These files are auto generated by VS Template and I did the huge mistake of instaling it in french, if I find time I will change it back to English. Anyway index.js and index.css can be removed, platformOverride.js gets regenerated each time (I need to check again though). I need to check again to remove the french comments from the templated parts.

DovydasNavickas commented 8 years ago

Awesome! Gonna wait for changes! :clap:

And with libraries, yes, the word I wrote is proper English, a plural version of the word library. But if I see it correctly here: image

It says librairies instead of libraries :smile: So, that would be the last change I'd recommend :clap: Good job! :smile:

pjeannet commented 8 years ago

It says librairies instead of libraries :smile: So, that would be the last change I'd recommend :clap: Good job! :smile:

Oups x)

pjeannet commented 8 years ago

Done!

DovydasNavickas commented 8 years ago

Nice! I think it's ready to merge :+1:

kevinchalet commented 8 years ago

Merci Pierre! :smile:

Do you wanna squash your commits before I merge it?

pjeannet commented 8 years ago

Done :)

kevinchalet commented 8 years ago

Merged, thanks Pierre! :clap:

https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Samples/commit/95857c5c2e6f741d7b91b7867b102ec7acfe8aff