Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

New payment card, switching apps, direct URLs, and more #641

Merged
merged 20 commits into from
Oct 31, 2019

Conversation

kyranjamie
Copy link
Contributor

@kyranjamie kyranjamie commented Oct 15, 2019

  • Removed Payment details notification in favour of changing button state
  • Using setState() method currently from parent component
  • Moved content/non-functional JSX to separate content.js file – I haven't seen this pattern followed elsewhere, but find it helps to focus on behaviour when visual noise is removed
  • Refactored maker/index.js to a functional component
  • Create slice of state for maker portal in store

@hstove as discussed, will use useReducer, but will do separately as this requires moving pages/maker/index.js to a function component w/ hooks

@hstove hstove temporarily deployed to app-co-staging-pr-641 October 15, 2019 12:54 Inactive
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from eb0fe30 to 6e93e58 Compare October 15, 2019 13:00
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 15, 2019 13:00 Inactive
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from 6e93e58 to b870dfd Compare October 15, 2019 14:07
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 15, 2019 14:07 Inactive
@kyranjamie kyranjamie requested a review from hstove October 16, 2019 11:57
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 16, 2019 12:03 Inactive
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 16, 2019 13:04 Inactive
Copy link
Collaborator

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Just a few small things, otherwise this is great and a solid increase in code health.

stores/maker/reducer.js Outdated Show resolved Hide resolved
@@ -0,0 +1,61 @@
import { keyBy } from 'lodash'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally, we've stuck to not importing helper libs like lodash in the front-end, to keep bundle sizes low. Additionally, this import will import the entire lib, which we definitely don't need. Two options here:

  1. Only import lodash/keyBy

  2. (Do this) look at you don't need lodash for an equivalent method if you need it.

Copy link
Contributor Author

@kyranjamie kyranjamie Oct 28, 2019

Choose a reason for hiding this comment

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

I did 1, and just notice you're pushing toward number 2.
Until app.co is at the level of maturity of having a test suite, wouldn't it better to use the lodash helper method, which itself is rigorously tested, than copy/pasting in a helper method with no tests or documentation?

@hstove hstove temporarily deployed to app-co-staging-pr-641 October 17, 2019 14:19 Inactive
pages/maker/index.js Outdated Show resolved Hide resolved
@hstove hstove had a problem deploying to app-co-staging-pr-641 October 21, 2019 17:52 Failure
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from 6bfd734 to 2e94aff Compare October 21, 2019 20:11
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 21, 2019 20:11 Inactive
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 21, 2019 20:17 Inactive
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 21, 2019 21:25 Inactive
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from 7c16076 to 446c839 Compare October 28, 2019 14:57
@aulneau aulneau self-requested a review October 28, 2019 17:20
components/maker/payment/helpers.js Show resolved Hide resolved
pages/maker/index.js Outdated Show resolved Hide resolved
pages/maker/index.js Outdated Show resolved Hide resolved
pages/maker/index.js Outdated Show resolved Hide resolved
pages/maker/index.js Outdated Show resolved Hide resolved
pages/submit.js Outdated Show resolved Hide resolved
stores/maker/actions.js Outdated Show resolved Hide resolved
stores/maker/actions.js Outdated Show resolved Hide resolved
stores/maker/selectors.js Outdated Show resolved Hide resolved
@hstove hstove changed the title feature/payment-details-card New payment card, switching apps, direct URLs, and more Oct 29, 2019
@hstove
Copy link
Collaborator

hstove commented Oct 29, 2019

Just gave this branch a look through. It seems like you pushed some half-finished code today, as the maker page doesn't build. It was just a few fixes to get it building, and I'm sure you just haven't pushed that yet. I also got the 'switching apps' functionality working, but it looks like it's not hydrating data yet. Nice work though, looking good overall!

Let's please get this branch in a working state by the demo tomorrow, so we can use the Heroku build for others to play with.

@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch 2 times, most recently from d0ef537 to 5224bad Compare October 29, 2019 13:16
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from 5224bad to b2bf0e4 Compare October 29, 2019 13:23
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from b2bf0e4 to faca69f Compare October 29, 2019 13:24
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 29, 2019 14:04 Inactive
@kyranjamie kyranjamie force-pushed the feature/payment-details-card branch from b3b6fc3 to 5a63a74 Compare October 29, 2019 14:24
@hstove hstove temporarily deployed to app-co-staging-pr-641 October 29, 2019 14:24 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants