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

Feature/refactor issuer entry point #86

Merged
merged 14 commits into from
Oct 11, 2018

Conversation

Grsmto
Copy link
Contributor

@Grsmto Grsmto commented Oct 10, 2018

Please make sure the following boxes are checked before submitting your Pull Request:

  • I linked the issues related to this PR in its description (if any).

  • If this PR adds new code that is not going to change in the near future, it includes unit tests to cover it.

  • Add new <ErrorBoundary> component (ref Handle users with unsupported browsers #56)

  • Refactor the way the app is structured. Now you can just open index.js and hopefully it should be clearer than before. PolymathUI.js and RouteLoader.js confusing components have been removed.
    note: This is not perfect yet because the code base is using the hacky react-router-config which makes things pretty confusing but from here it should be an easier step to switch router.

  • Fix styling missing on <Countdown> component + other loading of Sass styles issues

  • Fix a circular dependency that was breaking the build (there is still some others...)

the app now starts from index.js with single logic React tree
pass the data directly to the form handler instead of digging in the Redux state and import the form name from the caller
* develop:
  chore(offchain): add Heroku Procfile
  fix(offchain): don't log errors that are not defined
* develop: (62 commits)
  fix(offchain): fix mongodb env var name
  build(shared): remove dependency to old copied files
  docs: upgrade mongodb setup docs
  chore(shared): log command outputs
  chore(offchain): add testMatch pattern in jest config
  build(shared): improve local blockchain fixtures script
  fix: add escape hatch forceExit to finish tests in CI
  docs: updates testing docs
  test: setup test reporting
  chore: config lcov with cobertura for coverage reports
  test: generate coverage reports
  chore: remove extra test arguments from command
  chore: setup offchain deployments to heroku
  chore: update jest command to debug issues
  chore: enable detectOpenHandles option
  fix: remove duplicate jest dependency
  test(ui): update snapshots
  test(ui): update jest version
  test(ui): remove unused setup code
  docs: leave note about specs init script
  ...

# Conflicts:
#	packages/polymath-issuer/src/RouteLoader.js
#	packages/polymath-ui/src/index.js
dispatch: Function,
getState: GetState
) => {
const { email } = getState().form[confirmEmailFormName].values;
const { email } = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this eliminate the todo you placed above?
(BTW, redux form doesn't encourage this use in particular but rather the use of formValueSelector). Though for this use case what I say is irrelevant anyways :p.

I definitely agree with this change, we should be decoupling action creators from state wherever we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it doesn't because there is still one that needs to be refactored (I didn't to avoid breaking something) on line 8.
From what I see formValueSelector is to select a single value from the form, not to retrieve all of them. And even it would be a bad pattern. The concept itself of using Redux for forms encourage this kind of pattern. I actually never encountered a situation where I needed the form data to be in the global state (that's why community build other solutions like Formik).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I understand what you mean now, I recall now reading into this code I saw this as well.
I've never had to use the form state like this before, I'll read into Formik, what you said caught my attention ;)

componentDidCatch(error, errorInfo) {
// Display fallback UI
this.setState({ hasError: true });
// Raven.captureException(error, { extra: errorInfo });
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can. I left it because we are about to add support for Sentry in the next sprint (Raven = Sentry). Otherwise I agree I never leave stuff commented in general. What do you think?

Copy link
Contributor

@RafaelVidaurre RafaelVidaurre left a comment

Choose a reason for hiding this comment

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

Approved, I'd just remove that commented-out code unless there's a good reason for it to be there.

@RafaelVidaurre RafaelVidaurre merged commit 9731bff into develop Oct 11, 2018
@RafaelVidaurre RafaelVidaurre deleted the feature/refactor-issuer-entry-point branch October 11, 2018 17:36
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