Skip to content

Template, styles and behavior for bank-account component - #888

Merged
joshsmith merged 1 commit into
developfrom
860_payments_bank-account_(template+styles)
Dec 17, 2016
Merged

Template, styles and behavior for bank-account component#888
joshsmith merged 1 commit into
developfrom
860_payments_bank-account_(template+styles)

Conversation

@rlueder

@rlueder rlueder commented Dec 17, 2016

Copy link
Copy Markdown

What's in this PR?

Updates to bank-account component.

Notes

  • I used font-awesome just to get something quickly in place instead of editing our sprites
  • I added a click action to the first state of the page just so we can test the different states

TODO

  • Implement green check icon in icons sprite
  • Remove font-awesome
  • Remove test js
  • Add ember-stripe-service token generation
  • Add route action
  • Add button loading state

References

Fixes #860

@rlueder rlueder added this to the Launch Donations milestone Dec 17, 2016
@rlueder
rlueder requested a review from joshsmith December 17, 2016 05:21

@joshsmith joshsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I made some changes here.

Comment thread app/components/payments/bank-account.js Outdated

export default Component.extend({
classNames: ['bank-account']
classNames: ['bank-account__wrapper'],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we keep this named bank-account?

@joshsmith

Copy link
Copy Markdown
Contributor

@begedin I didn't get to it but the .account-setup classes should go in the account-setup.scss component template instead of in here. Those classes and the general HTML structure here is intended to be reused across the other components.

I can go back through and make the green check icon work. Don't replicate that across the other issues. I'll fix it here, and introduce the new styles elsewhere.

@joshsmith

Copy link
Copy Markdown
Contributor

These will also need the actual values bound to the real values, and the account number last 4 will have to be used here instead, along with minor things I'm probably not mentioning.

@begedin
begedin force-pushed the 860_payments_bank-account_(template+styles) branch from b7fee7f to a826450 Compare December 17, 2016 09:38
@begedin

begedin commented Dec 17, 2016

Copy link
Copy Markdown
Contributor

I did not use the route action for now, reason being I didn't want to potentially waste time and figuring out how to test that it's triggering properly. Instead, for now, the action is just passed out using closures.

We can switch to the route action helper when we have the time.

@begedin

begedin commented Dec 17, 2016

Copy link
Copy Markdown
Contributor

This took me longer than I thought due to me initially not realizing I need to implement the route action as well. I thought it would be just the component, not the whole logic chain, so I went for this before doing the recipient part in the other issue.

However, I got it as close as possible. The process consists of two steps - creating a token and adding the external account to the stripe connect account. Each step catches and handles it's own errors (a friendly string is displayed, similar to how it's done on the donate page.

Token creation was tested manually. Adding the account to our api record is untested and might need adjustments, but seems pretty straightforward for now.

An acceptance test will be need it. We can write it once we figure out the API parts and know the names of properties. Not sure if that's going to be part of this PR, or separate.

@begedin begedin changed the title template and styles for bank-account component Template, styles and behavior for bank-account component Dec 17, 2016
@joshsmith joshsmith assigned joshsmith and unassigned begedin Dec 17, 2016
Bind to properties and actions

Added error rendering, busy state, action handling at route level

Add icon for verified accounts

Fix project settings menu
@joshsmith
joshsmith force-pushed the 860_payments_bank-account_(template+styles) branch from 2ddca95 to 93a5da9 Compare December 17, 2016 19:28
@joshsmith
joshsmith merged commit f8bd3e5 into develop Dec 17, 2016
@joshsmith
joshsmith deleted the 860_payments_bank-account_(template+styles) branch December 17, 2016 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants