-
Notifications
You must be signed in to change notification settings - Fork 61
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
Improve playground with options #26
Conversation
A few things to talk @stevehobbsdev are around the package.json file for the playground. I'm using reactive forms and there's a special package I need to require as dependency for that, which shouldn't be part of the SDK contents. I think it would be best to create a new |
@@ -34,6 +34,7 @@ | |||
"@types/jasmine": "~3.5.0", | |||
"@types/jasminewd2": "~2.0.3", | |||
"@types/node": "^12.12.53", | |||
"@angular/forms": "^10.0.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only used on the playground
@@ -1,54 +1,83 @@ | |||
<!-- Next Steps --> | |||
<h1>{{ title | uppercase }}</h1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to have a property for this
|
||
beforeEach(async(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no async code runs inside this
}); | ||
|
||
xit('should logout with default options', () => { | ||
//TODO: uncheck all checkboxes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand to set up this test case we should be unchecking the option checkboxes
|
||
const butLogout = compiled.querySelector('#logout'); | ||
expect(butLogout.disabled).toBeFalse(); | ||
//TODO: assert logout() was called without options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here, check that the SDK's logout method was called
isAuthenticated$ = this.auth.isAuthenticated$; | ||
isLoading$ = this.auth.isLoading$; | ||
user$ = this.auth.user$; | ||
accessToken$ = new BehaviorSubject(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to keep the "manually" retrieved access token,
this.auth.getAccessTokenWithPopup(), | ||
this.auth.getAccessTokenSilently() | ||
) | ||
.pipe(first()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runs once and unsubscribes
imports: [ | ||
BrowserModule, | ||
AppRoutingModule, | ||
ReactiveFormsModule, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one was added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good, and I agree with defaulting returnTo
on logout. If you feel like making that change, feel free to merge.
@stevehobbsdev fixed the remaining unit tests, added the checkbox we talked about, and re-enabled the lint test command for this playground. If you're happy with the changes, good to merge |
Description
Testing
I've added unit tests for the components and some properties. There are some tests that are marked as ignored with
xit
until I find out how to modify the form on the template before calling the associated action.Checklist
master
Screenshots
Authenticated
NOT Authenticated