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

HistoryStore for tracking relevant routes #4305

Merged
merged 4 commits into from
Jan 26, 2017
Merged

HistoryStore for tracking relevant routes #4305

merged 4 commits into from
Jan 26, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 26, 2017

Split from https://github.com/ethcore/parity/pull/4178

  • stores navigation entries in localstorage
  • currently enabled for dapps & accounts
  • /home uses these to display the recently-used

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 26, 2017
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 26, 2017
@jacogr jacogr mentioned this pull request Jan 26, 2017
Merged
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jan 26, 2017
jacogr added a commit that referenced this pull request Jan 26, 2017
@jacogr
Merge #4305
85c9c89
jacogr
jacogr commented Jan 26, 2017
View reviewed changes
js/src/routes.js
@@ -16,11 +16,15 @@

import {
Accounts, Account, Addresses, Address, Application,
Contract, Contracts, Dapp, Dapps,
Contract, Contracts, Dapp, Dapps, HistoryStore,
Copy link
Contributor Author

@jacogr jacogr Jan 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not 100% convinced on this location as ~/views/historyStore.js. It is only related to keeping a history of views so ~/views makes sense in a weird way, but I'm open to suggestions to other locations for the sake of consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better in ~/util ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My knee-jerk reaction was that ~/util is even worse, my thoughts after 30s is that it may fit. (Although it really is meant for swiss-army-knife utilities)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep that's what I think too but it's true that it feels a bit strange/out-of-place in ~/views... @derhuerst any thoughts?

@ngotchac ngotchac added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 26, 2017
@ngotchac
Copy link
Contributor

mustntgrumble only related to the HistoryStore path. Looks good otherwise !

@jacogr
Copy link
Contributor Author

jacogr commented Jan 26, 2017

Leaving the path for now - I'm actually hoping another store ends up being the same that way will move it in ~/stores or (probably better) refactor the ~/redux stuff properly.

@jacogr jacogr merged commit 7f3b1bd into master Jan 26, 2017
@jacogr jacogr deleted the jg-historystore branch January 26, 2017 16:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ngotchac ngotchac ngotchac left review comments

Assignees
No one assigned
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jacogr @ngotchac