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.

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

Updated the vanilla JS demo. #140

Merged
merged 1 commit into from
Mar 3, 2018
Merged

Updated the vanilla JS demo. #140

merged 1 commit into from
Mar 3, 2018

Conversation

itrew
Copy link
Contributor

@itrew itrew commented Mar 1, 2018

Per discussion #138 (comment) this is an updated version of the vanilla js demo. Addresses #11 and should close #100. This implementation stores the session in localStorage and de-serializes it into a new session when not using a popup.

The general gist of OAuth is included, but with this being vanilla I wasn't going to go through the trouble of adding the same search functionality that I had added to the VueJS demo.

image

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 568a7ed on itrew:vanilla-js-demo into e2b05b2 on Esri:master.

@Esri Esri deleted a comment from coveralls Mar 1, 2018
@Esri Esri deleted a comment from coveralls Mar 1, 2018
@Esri Esri deleted a comment from coveralls Mar 1, 2018
@itrew
Copy link
Contributor Author

itrew commented Mar 1, 2018

Realized when the session was being placed into local storage the expiration date was being stringified. Last commit now creates a new date from that string when parsing the serialized session on page load.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

this looks great. proposed a few small changes in itrew#1

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

thanks again! 👍

Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

@itrew This generally looks good but I have a few nitpicky things that @jgravois or you should take care of before this gets merged.

<script src="node_modules/@esri/arcgis-rest-auth/dist/umd/arcgis-rest-auth.umd.js"></script>
<script>
const match = window.location.href.match(
/clientID=(.+)#/
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a comment here that this should usually be hardcoded.

session = arcgisRest.UserSession.completeOAuth2({
clientId,
});
localStorage.setItem('__ARCGIS_REST_USER_SESSION__', JSON.stringify(session));
Copy link
Contributor

Choose a reason for hiding this comment

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

session has a serialize method that could be used here like session.serialize() we should show that instead https://esri.github.io/arcgis-rest-js/api/auth/UserSession/#serialize.

const clientId = match[1];
let session;
function processAuthentication() {
session = arcgisRest.UserSession.completeOAuth2({
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably come AFTER the local storage and window calls since it will call window.close() in somecases.

Copy link
Contributor

Choose a reason for hiding this comment

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

i moved the window call, but we can't call session.serialize() if the object isn't instantiated yet.

@patrickarlt patrickarlt mentioned this pull request Mar 3, 2018
@jgravois
Copy link
Contributor

jgravois commented Mar 3, 2018

pushed another commit to incorporate @patrickarlt's (good) suggestions and rebased.

@jgravois jgravois merged commit 1da932d into Esri:master Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor demos/vanilla sample to demonstrate popup:false as well
4 participants