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 #363

Merged
merged 88 commits into from
Nov 29, 2017
Merged

(Refactor) Mobx state management #363

merged 88 commits into from
Nov 29, 2017

Conversation

fvictorio
Copy link
Contributor

This is a continuation of #304, with the merge conflicts resolved. I created a new PR because I can't push to @15chrjef repo.

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 24, 2017

@fvictorio @fernandomg
2.
image 2017-11-24 16-48-16

  1. Contributors amount is not empty here: https://wizard.oracles.org/crowdsale?addr=0xa3f54827dE7AC856762F77A0A9701548170e07D6&networkID=1
    But from this branch amount of contributors is 0.

@vbaranov vbaranov self-requested a review November 24, 2017 15:03
Copy link
Collaborator

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

Looks good for me now

@igorbarinov
Copy link
Member

@fvictorio @fernandomg great job.

  • I think we should add a document to Wiki with the state structure and how-to for new developers
  • let's start release policy with this important and large PR. How do you feel about release 0.1?

@fvictorio
Copy link
Contributor Author

0.1 sounds good.

I'll add some documentation in the wiki for new developers, but I don't think documenting the state structure is worth it: it's going to get old fast, and the MobX stores seem pretty self-documenting. What do you think?

@igorbarinov
Copy link
Member

If you can document basic ideas of our state to new developers it would be great
I agree with you about state structure if it will be old soon

@vbaranov
Copy link
Collaborator

vbaranov commented Nov 24, 2017

I agree with documenting of the structure. Day will come, when we'll forgot, where some properties for wizard are stored, if we won't describe it.

@fvictorio
Copy link
Contributor Author

I added a page to the wiki: https://github.com/oraclesorg/ico-wizard/wiki/Source-code-structure

I didn't describe all stores because I think some of them should be replaced with local state.

@igorbarinov
Copy link
Member

@fvictorio could we deploy it on stage server? At the moment the way we did it is to merge a PR into stage branch?

Do you have any other ideas for staging?

@fvictorio
Copy link
Contributor Author

No, that sounds perfect. I'll merge it on the stage branch.

@fvictorio
Copy link
Contributor Author

Done, it is in the stage server now. I am testing it there.

@igorbarinov
Copy link
Member

@fvictorio thank you, will test too

@vbaranov Viktor, btw what was the idea behind syncing to branches using PRs? E.g. #368
If there is no special reason let's sync branches without PRs.

@fvictorio
Copy link
Contributor Author

It seems to be working fine. I tested crowdsales with token reservation, multiple tiers, whitelisting.

@fvictorio
Copy link
Contributor Author

Hi, what's the next step here? Do we leave it in stage some time more for testing or can we merge it into master?

Copy link
Member

@igorbarinov igorbarinov left a comment

Choose a reason for hiding this comment

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

Tested basic scenario on the staging server and it worked fine

@vbaranov vbaranov merged commit 6b41327 into master Nov 29, 2017
@ghost ghost removed the awaiting for review label Nov 29, 2017
@igorbarinov
Copy link
Member

Congrats everyone on PR merge!

@fvictorio could you please create a tag and release the source code in it with brief description of MobX changes.

@fvictorio
Copy link
Contributor Author

Sure!

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.

None yet

5 participants