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

(Refactor) Mobx state management #304

Merged
merged 81 commits into from
Nov 29, 2017
Merged

Conversation

mark-antony1
Copy link
Contributor

This is a huge commit that will require significant time dedicated to testing and resolving merge conflicts. It is a work in progress and is planned to be updated often until it is finally merged in.

High Level Changes

  • Mobx state management
  • removal of component state management
  • some small cleanup and refactoring where appropriate
  • pricing strategy integrated into the relevant information of its corresponding tier

More Detailed Changes

Ejected create-react-app for access to babel, which allows us to use decorators.
Babel file was based upon current mobx documentation.
Updated naming convention of all stores. Each store's name should ideally have 'store' within it to draw less confusion when importing stores into components.
I updated the initialized item name and the class itself.
Added an index.js files for all stores, and imported the stores into App.js. Stores are passed into the Provider method of 'mobx-react'.
Added a build_scripts folder that will hold build, start, and test files.
Things to test

Ensure that the ejection from create-react-app will not obstruct our build process.
Check to see that the provider component has access to the stores. This can be easily done through react-devtools, by checking that the props contain an object of stores.

@igorbarinov igorbarinov changed the title (refactor) Mobx state management (Refactor) Mobx state management Oct 27, 2017
@vbaranov
Copy link
Collaborator

@15chrjef use this package.json for Mobx PR

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 1, 2017

@15chrjef you can merge my PR to your branch to make deployment stage of Wizard working

@mark-antony1
Copy link
Contributor Author

Thanks @vbaranov. It works with the crowdsale through Kovan on the Invest and crowdsale page.

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 2, 2017

@15chrjef one bug still exists: for [Tier name]: is lost.

From the downloaded at step 4 file:

  1. Crowdsale contract address0xB1FF1C4D7BfaF3aeFe4eF1e3aB604b7F2b28410f -> Crowdsale contract address for [Tier name]: 0xB1FF1C4D7BfaF3aeFe4eF1e3aB604b7F2b28410f
  2. Pricing strategy contract address0x3D8E0Fa030a44e77F9C4738AEE843752d79bA1FD -> Pricing strategy contract address for [Tier name]: 0x3D8E0Fa030a44e77F9C4738AEE843752d79bA1FD
  3. Finalize agent contract address0x2Aeb492CEEA0aF88E1EAf681f0578886eBaA7372 -> Finalize agent contract address for [Tier name]: 0x2Aeb492CEEA0aF88E1EAf681f0578886eBaA7372

@mark-antony1
Copy link
Contributor Author

Where do you get these errors? What prompts it?

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 2, 2017

@15chrjef It is from downloaded file at step 4. Try to download and compare with that from master branch

sdzharkov and others added 2 commits November 3, 2017 13:37
Fix Continue button not showing Errors
@fvictorio
Copy link
Contributor

fvictorio commented Nov 13, 2017

I looked into this PR and here's what I thing we should do:

  1. Merge the other 3 open PRs first, otherwise is going to be extra hard. (Feature) Payment proccess views #199 and (Feature) Validate addresses used in reserved tokens #358 are ready IMO; (Feature) UI Improvements in Invest page #341 depends on (Feature) Payment proccess views #199 and needs a proper review when that one is merged, but it's only view layer stuff so it shouldn't take long. Any non-critical problems with those PRs should go to open issues and be taken care of after this MobX PR is merged.
  2. Solve the conflicting files very carefully. Probably @15chrjef is the best person to do it, but if he's busy, I can give it a try. What I'd do is to use the files with conflicts and re-apply the MobX changes by hand, because several things have changed since the last commit in the merged branch, and because solving each conflict by hand is very error prone.

I think this PR is very important for improving the codebase, and the longer we wait, the harder it will be to do it. If there's nothing of much higher priority, I'd focus our efforts this week in getting this merge out of the way.

@vbaranov vbaranov merged commit 785bd57 into poanetwork:master Nov 29, 2017
@ghost ghost removed the in progress label Nov 29, 2017
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.

5 participants