-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
MSAL-Angular Angular13 Support & RxJS 6/7 Sample Apps #4605
Conversation
Codecov Report
*This pull request uses carry forward flags. Click here to find out more. |
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.
Tested locally and this looks good, thanks! I've push up one commit to remove the package-lock
files.
Please complete the following:
- Generate change file (can mark as minor change)
- Add both of these samples to the e2e tests (@jo-arroyo can help you get that running)
- Add
13
as supported verison in the supported version table in the MSAL Angular README - As fast follow up to this PR, start another PR to add an Angular 14 sample.
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.
Also tested locally. Great work! A few more changes would be nice:
- Update the READMEs for each sample, cf. Angular 10 sample.
- Update the README for the
msal-angular-v2-samples
folder. This is where we list the differences between the samples. Feel free to include an explanation for why we have a RxJS 6 and RxJS 7 sample. - Update the MSAL Angular README to include links to angular13 samples.
- Remove the
.vscode
andassets
folders for each sample.
I will reach out to you offline about the e2e tests.
…de and assets folders from both Angular13 samples. Added the Angular 12 sample's e2e tests to the Angular 13 samples - they do not run yet.
change/@azure-msal-angular-8f602981-ceaf-457b-9710-7afdfeb9fcae.json
Outdated
Show resolved
Hide resolved
…e.json Co-authored-by: Jason Nutter <janutter@microsoft.com>
@Robbie-Microsoft E2E tests are passing. It required that I add Lines 25 to 28 in 94f17e2
It would probably be good to call this out for each sample. |
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.
Looks good, thanks for putting this together!
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.
A few changes, thanks!
samples/msal-angular-v2-samples/angular13-rxjs7-sample-app/README.md
Outdated
Show resolved
Hide resolved
samples/msal-angular-v2-samples/angular13-rxjs6-sample-app/README.md
Outdated
Show resolved
Hide resolved
samples/msal-angular-v2-samples/angular13-rxjs7-sample-app/jest.config.js
Outdated
Show resolved
Hide resolved
samples/msal-angular-v2-samples/angular13-rxjs6-sample-app/jest.config.js
Outdated
Show resolved
Hide resolved
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.
Looks great! Thanks for all your good work!
No description provided.