Skip to content

Conversation

@wlee221
Copy link
Contributor

@wlee221 wlee221 commented Nov 4, 2021

Issue #, if available:

Description of changes: Add service prop to amplify-sign-up, which enables customers to provide their own validation services. Also adds amplify-checkbox component 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.

@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2021

🦋 Changeset detected

Latest 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

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2021

This pull request introduces 1 alert when merging 764ddc6 into c749e50 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 00:53 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 00:53 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 00:53 Inactive
@wlee221 wlee221 marked this pull request as ready for review November 4, 2021 16:42
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 16:54 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 16:54 Inactive
@@ -0,0 +1,8 @@
---
Copy link
Contributor Author

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)}`;
Copy link
Contributor Author

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.

Copy link
Contributor Author

@wlee221 wlee221 Nov 4, 2021

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.

@wlee221 wlee221 temporarily deployed to ci November 4, 2021 17:34 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 17:34 Inactive
Copy link
Contributor

@ericclemmons ericclemmons left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

@wlee221 wlee221 Nov 4, 2021

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.

Copy link
Contributor Author

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.

Comment on lines +23 to +30
<amplify-checkbox
[errorMessage]="validationErrors.acknowledgement"
[hasError]="!!validationErrors.acknowledgement"
name="acknowledgement"
value="yes"
>
I agree with the Terms & Conditions
</amplify-checkbox>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sliiiiiiick

@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:16 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:16 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:16 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:36 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:36 Inactive
@wlee221 wlee221 temporarily deployed to ci November 4, 2021 18:36 Inactive
@wlee221 wlee221 enabled auto-merge (squash) November 4, 2021 18:49
@wlee221 wlee221 merged commit ef1e0f0 into main Nov 4, 2021
@wlee221 wlee221 deleted the angular/custom-sign-up branch November 4, 2021 18:58
@github-actions github-actions bot mentioned this pull request Nov 4, 2021
@wlee221 wlee221 self-assigned this Nov 4, 2021
thaddmt pushed a commit that referenced this pull request Apr 7, 2023
Sync amplify-ui to amplify-ui-staging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants