Skip to content
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

feat(@aws-amplify/ui-components): Framework Bindings for Authen… #4676

Merged

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Jan 7, 2020

[Delivers #170557495]

This supersedes #4340, which had too many conflicts to bother with.

Scope

  • Adds framework bindings for Angular (@aws-amplify/ui-angular), React (@aws-amplify/ui-react), and Vue (@aws-amplify/ui-vue).
  • Does not solve stylistic differences & specificity problems.
  • Does not solve event/bus/hub differences.
  • Does not introduce HoCs.

Building

  1. yarn workspace @aws-amplify/ui-components build
  2. yarn workspace @aws-amplify/ui-react build
  3. yarn workspace @aws-amplify/ui-angular build
  4. yarn workspace @aws-amplify/ui-vue build

Demo

See: https://github.com/aws-amplify/amplify-js-samples-staging/pull/39

Action Items

  • Create bindings as @aws-amplify/ui-react.

  • Stories to validate <AmplifyAuthenticator /> usage for React.

    Screen Shot 2020-01-07 at 10 58 12 AM
  • Create bindings as @aws-amplify/ui-angular.

  • Stories to validate <AmplifyAuthenticator /> usage for Angular.

  • HTML samples compat (@jordanranz).

  • React samples compat.

    Styles should be resolved in another PR:

    Screen Shot 2020-01-08 at 4 38 25 PM

    Functionality will need to be fixed in another PR:

    amplify-authenticator-error
  • Angular samples compat.

    Styling to be addressed, but <amplify-authenticator> is practically a drop-in replacement:

    Screen Shot 2020-01-10 at 10 18 38 AM
  • Bring over AmplifyService from aws-amplify-angular, but to what degree?

  • Vue samples compat.

    Screen Shot 2020-01-10 at 10 43 08 AM
  • Create @aws-amplify/ui-vue.

  • v-bind:federated="federated" passes [Object object]. The solution is to use .prop via :federated.prop="federated":

    Screen Shot 2020-01-10 at 11 46 28 AM
  • Remove extraneous dependencies originally found in from https://github.com/ionic-team/stencil-ds-plugins-demo/ and replace build process with the one unused for the other aws-amplify packages. Going to minimize dependencies, but not use the same build.js script due to a huge amount of TypeScript errors 😱

Notes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ericclemmons ericclemmons added React React related issues UI Related to UI Components labels Jan 7, 2020
@ericclemmons ericclemmons self-assigned this Jan 7, 2020
@ericclemmons ericclemmons changed the title feat(@aws-amplify/ui-components): React Bindings for Authenticator feat(@aws-amplify/ui-components): Framework Bindings for Authenticator Jan 7, 2020
@ericclemmons
Copy link
Contributor Author

Can't do stories for Angular, primarily since it lacks a run-time that I could use as a decorator (the same way I'm rendering React within HTML).

The ideal solution is part of 6.0.0 😱:

storybookjs/storybook#3889

Instead, I'll have to keep experimenting with the bindings output (+ the production output) and yarn link it to test with our samples repo:

https://github.com/ionic-team/stencil-ds-plugins-demo/tree/master/packages/component-library-angular

@aws-amplify aws-amplify deleted a comment from lgtm-com bot Jan 10, 2020
@aws-amplify aws-amplify deleted a comment from lgtm-com bot Jan 10, 2020
@aws-amplify aws-amplify deleted a comment from lgtm-com bot Jan 10, 2020
@aws-amplify aws-amplify deleted a comment from lgtm-com bot Jan 10, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jan 10, 2020

This pull request fixes 1 alert when merging 2f608de into 1aa4034 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@ericclemmons
Copy link
Contributor Author

@jordanranz Looking through the PR and I was able to reduce the "noise" via .gitattributes, but require #4705 to really get it down.

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #4676 into ui-components/master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                  @@
##           ui-components/master   #4676   +/-   ##
====================================================
  Coverage                  78.6%   78.6%           
====================================================
  Files                       164     164           
  Lines                      8819    8819           
  Branches                   1765    1765           
====================================================
  Hits                       6932    6932           
  Misses                     1747    1747           
  Partials                    140     140

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72b269a...59f9c33. Read the comment docs.

…-js into 170557495-bindings

# Conflicts:
#	packages/amplify-ui-components/src/components.d.ts
#	yarn.lock
@lgtm-com
Copy link

lgtm-com bot commented Jan 13, 2020

This pull request fixes 1 alert when merging ceee528 into bc0ae89 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@ericclemmons ericclemmons marked this pull request as ready for review January 13, 2020 17:30
@ericclemmons
Copy link
Contributor Author

Alright, #4705 got this down to the essentials! package.json updates, and a bit of configuration.

@lgtm-com
Copy link

lgtm-com bot commented Jan 14, 2020

This pull request fixes 1 alert when merging 72b269a into b617735 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

Copy link
Contributor

@jordanranz jordanranz left a comment

Choose a reason for hiding this comment

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

LGTM

},
"scripts": {
"build": "npm run build.ng",
"build.link": "npm run build && node scripts/link-copy.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious on the . vs :

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shamelessly copy/pasted from Stencil's demo, as their Angular example relies on custom, noisy scripts to work :(

I'm fine with changing to : to match our other conventions, however. I anticipated this would change once we adapted our ./build.js script for convention, but I passed on that for this PR because of the level of effort involved.

packages/amplify-ui-angular/package.json Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jan 15, 2020

This pull request fixes 1 alert when merging 9b85672 into 1d568b6 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Jan 15, 2020

This pull request fixes 1 alert when merging 59f9c33 into 1d568b6 - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@ericclemmons ericclemmons changed the title feat(@aws-amplify/ui-components): Framework Bindings for Authenticator feat(@aws-amplify/ui-components): Framework Bindings for Authen… Jan 15, 2020
@ericclemmons ericclemmons merged commit 407089f into aws-amplify:ui-components/master Jan 15, 2020
@ericclemmons ericclemmons deleted the 170557495-bindings branch January 15, 2020 22:02
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Angular Related to Angular 2+ React React related issues UI Related to UI Components Vue Related to Vue Framework issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants