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

Added stripe element and event to actions #8

Conversation

ivanvanderbyl
Copy link
Contributor

This PR adds support for sending the stripeElement and event object as arguments to the event handlers, as well as adding new actions:

  • error which fires only when the change event fires and the card is invalid. This can be used in a similar way to complete for more complex error state handling, like disabling a form submit or show error messages outside the block.
  • complete which only fires once the card input is complete. This allows for sending the stripeElement up to another form object or controller to handle complex nested payment forms. For example:
{{#form-for changeset submit=(action "createCustomer") as |f|}}
  {{stripe-card complete=(mut changeset._stripeElement)}}
  {{f.text-field "billingEmail" placeholder="[email protected]" label="Send receipts to:"}}
  {{f.submit "Begin Subscription"}}
{{/form-for}}
createCustomer(changeset) {
  return changeset.validate().then(() => 
    if(changeset.get('isValid')) {
      stripe.createToken(changeset.get('_stripeElement')).then(({token}) => {
        set(changeset, 'cardToken', token);
      });
      ...
    }
  });
}


if (complete === true) {
this.sendAction('complete', stripeElement);
}else if(error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

} else if (error) {

Also, is it really complete || error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe it should just be the default action? Also, do you have an aversion to prefixing event based actions with on-?

@joshsmith
Copy link
Contributor

@ivanvanderbyl thanks for this PR!

Overall this solves some of the design issues with the blocked usage.

A few things I would love to see here:

  • Some additions to the README that mention this usage
  • Possibly some additions to the dummy app to also show off what kind of complex behavior can emerge
  • Mentioning usage of yarn in the README
  • If you have any thoughts around relevant tests

Although not relevant to the PR specifically, I also noticed we should probably have:

  • eslint that runs on Travis, so we could've caught the issue with the head tag insertion, and the spacing comment I added

@joshsmith
Copy link
Contributor

I started in #7 getting coverage working, so it might be nice to wrap that up first if we can discuss some testing ideas (being that the iframe makes much of this difficult). With testing wrapped up, I'll feel much more comfortable about such large surface area changes to the actions/events.

@ivanvanderbyl
Copy link
Contributor Author

Cool, yep I'll update the readme and address those other points once I get this feature shipped in our app.

With the block usage, the main hurdle design wise is lacking any way to specify where the Stripe Element is placed. Maybe we could yield a sub-component which just renders the placeholder div where it's needed, but then maybe that isn't a design concern for this addon. Wouldn't want to complicate it unnecessarily.

@ivanvanderbyl
Copy link
Contributor Author

Testing the iframe could be tricky, but i have seen something which did this before, I think it was outside of the ember landscape though.

@joshsmith
Copy link
Contributor

@ivanvanderbyl awesome to hear! Are you in the Ember slack? Or you could join ours http://slack.codecorps.org

@joshsmith
Copy link
Contributor

Hey @ivanvanderbyl just wanted to check in on this with you.

@joshsmith
Copy link
Contributor

Hi @ivanvanderbyl I would still love to see about this getting in. Have you had any time to spend on this?

@joshsmith
Copy link
Contributor

Ping.

@ivanvanderbyl ivanvanderbyl force-pushed the added-stripe-element-and-event-to-actions branch from e707814 to aa9d88b Compare August 22, 2017 03:45
@joshsmith joshsmith force-pushed the added-stripe-element-and-event-to-actions branch 2 times, most recently from c261ece to 77c21a4 Compare November 10, 2017 21:45
- Schedule running Elements setup after insert to avoid rerender loop
- Add yarn.lock
- Include Stripe Element and event in actions
- Add complete action and send error
- Add eslint
- Fix eslint errors and remove phantomjs
- Update travis.yml
- Update ember-try
- Add testem args for chrome
@joshsmith joshsmith force-pushed the added-stripe-element-and-event-to-actions branch from 36bdfd1 to 5e9478b Compare November 11, 2017 01:32
@joshsmith
Copy link
Contributor

Opened #17 as a follow-up to this since I'd like to get this merged.

Thanks again for the PR @ivanvanderbyl! 🙌

@joshsmith joshsmith merged commit 60ecfc4 into code-corps:master Nov 11, 2017
@ivanvanderbyl ivanvanderbyl deleted the added-stripe-element-and-event-to-actions branch November 13, 2017 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants