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

Add react router #194

Closed
wants to merge 8 commits into from
Closed

Conversation

dmihal
Copy link
Collaborator

@dmihal dmihal commented May 28, 2019

Reissuing #144

@dmihal dmihal changed the base branch from reactrouter to dev May 28, 2019 09:34
@dmihal dmihal changed the base branch from dev to master May 29, 2019 11:45
@dmihal dmihal changed the base branch from master to dev May 29, 2019 11:45
@TimDaub
Copy link
Collaborator

TimDaub commented Jun 4, 2019

Hi @dmihal 👋

There's awesome stuff in this PR. In particular, I see the following things happening:

  • creating a config.js
  • refactoring RecentTransactions into a TransactionStore
  • add react-router-dom.

To speed up the review, I'd suggest however that we look at the above mentioned pieces individually. Would you mind splitting them up into individual PRs?

Regarding config.js, we've done something very similar here: https://github.com/leapdao/burner-wallet/blob/master/src/config.js

Right now, I'd prefer our solution to the one proposed here as it

  • takes into account most magic constants in the code (e.g. we've taken them out of Exchange.js)
  • generically renames the constants and groups them (e.g. SIDECHAIN and ROOTCHAIN).
  • doesn't overwrite already defined constants based on location (e.g. POA_XDAI_NODE).

If you don't mind, I'd prepare a PR for changing config.js and sending it here soon for e.g. you to review. Thoughts?

@dmihal
Copy link
Collaborator Author

dmihal commented Jun 4, 2019

Hey @TimDaub

This PR was recreating #144, which got removed from master after a big rollback before one of the burner events.

Looks like a couple of the other PRs I made back then got swept up in this one (TransactionStore, config.js).

At this point I'm mostly focusing on building the new structure for the burner wallet. Rebasing those old PRs into the current master was a bit difficult and I don't think i have the time to focus on splitting apart those old PRs.

Right now I'm open to either letting this PR close or someone else take over this task, but open to other thoughts.

@TimDaub
Copy link
Collaborator

TimDaub commented Jun 4, 2019

@dmihal OK, no problem! I'll take care of it then.

@TimDaub
Copy link
Collaborator

TimDaub commented Jun 5, 2019

I was thinking whether or not to simply untangle this PR myself today but then I became hesitant about the time that would be required. I'm also unsure about having this functionality in master, given that we're now working according to the spec. So I've decided to do the following:

@dmihal if you have a better idea, please let me know.

@TimDaub TimDaub closed this Jun 5, 2019
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