- Notifications
You must be signed in to change notification settings - Fork 342
feat(angular): Add services prop and custom-sign-up example #628
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
Conversation
🦋 Changeset detectedLatest commit: e61a7d3 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| This pull request introduces 1 alert when merging 764ddc6 into c749e50 - view on LGTM.com new alerts:
|
...c/pages/ui/components/authenticator/custom-sign-up-fields/custom-sign-up-fields.component.ts Outdated Show resolved Hide resolved
| @@ -0,0 +1,8 @@ | |||
| --- | |||
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.
Added two changesets because this PR because this PR does two different things. Let's see how #596 consumes this.
| @Input() autocomplete = 'new-password'; | ||
| @Input() disabled = false; | ||
| @Input() id: string; | ||
| @Input() fieldId: string = `amplify-field-${nanoid(12)}`; |
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.
Renamed this prop id to fieldId to prevent duplicate id html attribute values on screen, which breaks accessibility.
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.
Related, angular/angular#18877 -- this wouldn't be a problem if Angular support fragments.
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.
Nothing blocking, looking good!
| 'docs': patch | ||
| 'angular-example': patch | ||
| '@aws-amplify/ui-angular': patch | ||
| 'e2e': patch |
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.
Should docs, angular-example, & e2e be ignored?
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.
Yeah, I'm divided on this. Note that this is what shows up by default when you click the "add changeset" link from the bot and these will be ignored on changeset version or changeset publish as per .changeset/config.json.
I personally don't like the DX of having to remove these ignored packages manually, so I kept it as-is. Let's seee how this gets consumed in #596 and we can have a DX discussion based off that.
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.
Ahhhhhh https://github.com/aws-amplify/amplify-ui/runs/4109281658?check_suite_focus=true
🦋 error Error: Found mixed changeset wicked-cups-tie
🦋 error Found ignored packages: docs angular-example e2e
🦋 error Found not ignored packages: @aws-amplify/ui-angular
We do need to remove those unused packages. TIL! I'll just push something to main directly to fix that shortly.
| <amplify-checkbox | ||
| [errorMessage]="validationErrors.acknowledgement" | ||
| [hasError]="!!validationErrors.acknowledgement" | ||
| name="acknowledgement" | ||
| value="yes" | ||
| > | ||
| I agree with the Terms & Conditions | ||
| </amplify-checkbox> |
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.
Sliiiiiiick
docs/src/pages/ui/components/authenticator/customization.angular.mdx Outdated Show resolved Hide resolved
Sync amplify-ui to amplify-ui-staging
Issue #, if available:
Description of changes: Add
serviceprop toamplify-sign-up, which enables customers to provide their own validation services. Also addsamplify-checkboxcomponent for customers to use.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.