-
Notifications
You must be signed in to change notification settings - Fork 316
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
fix(examples,e2e): Add examples and tests for #1580 #1578
fix(examples,e2e): Add examples and tests for #1580 #1578
Conversation
|
This pull request introduces 3 alerts when merging 7aee0cf into eedae23 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 81492e1 into eedae23 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging a6e26ea into 7e00926 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 5a090b8 into 7e00926 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 3e3527f into 7e00926 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 4ab0271 into 7e00926 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging 5a62d1d into 7e00926 - view on LGTM.com new alerts:
|
This pull request introduces 3 alerts when merging c9cf401 into fc71b28 - view on LGTM.com new alerts:
|
<!-- TODO: this authenticator shouldn't be needed --> | ||
<authenticator></authenticator> |
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.
Vue still needed this hidden authenticator
, because authenticator was needed to interpret and start the machine in the first place. This seemed like a much bigger refactor, so left this as-is.
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.
+1 I can look at this in a different PR. Maybe we need to add an initialization via provide/inject somewhere. A user could also hide this with CSS when the user is not logged in, I suppose
This pull request introduces 3 alerts when merging bae13aa into e777730 - view on LGTM.com new alerts:
|
|
||
Then('I see a valid greetings message', () => { | ||
cy.findByRole('document') | ||
.contains(new RegExp('^Hello, .+!$', 'i')) |
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.
Checks if the message has "Hello, {username}" structure. Kept it generic to not expose the test username 😈
@@ -101,6 +102,10 @@ const routes: Routes = [ | |||
path: 'ui/components/authenticator/useAuthenticator', | |||
component: UseAuthenticatorComponent, | |||
}, | |||
{ |
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.
Nice!
export default function MyApp(props) { | ||
return ( | ||
<AmplifyProvider> | ||
<Authenticator.Provider> |
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 was testing this in the other PR and I had to add this. Glad it's there now.
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.
LGTM!
This pull request introduces 3 alerts when merging 16aefaf into 7857497 - view on LGTM.com new alerts:
|
…1580) * [Revert] use devTools * Check for current user first before init * Revert "[Revert] use devTools" This reverts commit df53b24. * react: send INIT event on 'setup' state * vue: send INIT event on 'setup' state * angular: send INIT event on 'setup' state * Remove unused var * Move comment * Create five-crabs-help.md * Update packages/ui/src/machines/authenticator/index.ts Co-authored-by: Caleb Pollman <cpollman@amazon.com> * Combine if statements * Unsubscribe on vue * Unsubscribe on angular * fix(examples,e2e): Add examples and tests for #1580 (#1578) * Angular Example * React example * Vue examples * Add missing ! * Update useAuthenticator test * Use slots on parent route * Configure Amplify on `/home` * Add react sign out button * Update wording on react example * Add sign out test * Add angular sign out buttons * Sign Out button in Vue * destructure signOut ref * Add hidden Authenticator on Vue * Fix copy paste typo * fix(e2e): AutoSignIn directly after signUp is done * Trigger CI Co-authored-by: Caleb Pollman <cpollman@amazon.com>
Description of changes
This updates the
useAuthenticator
examples and their e2e test to match #1580.Details
Our current
useAuthenticator
examples are really SPA and do not sufficiently test router use cases.There are many
useAuthenticator
bugs, especially with refresh, so we plan to update examples and tests incrementally as we add morerouter
support. For now, this PR adds additional routeuseAuthenticator/home
.This PR adds:
/useAuthenticater
route. This has the authenticator for user to sign in.Future Goals
This example needs to be extended to better resemble customer use cases:
/sign-in
route/home
and make it actually protected.Issue #, if available
Related to #1332, #1497, accompanies #1580
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.