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

docs(examples): add url sync #1542

Merged
merged 1 commit into from
Nov 14, 2016
Merged

docs(examples): add url sync #1542

merged 1 commit into from
Nov 14, 2016

Conversation

mthuret
Copy link
Contributor

@mthuret mthuret commented Nov 14, 2016

uses browser history and a 700 ms threshold.

the url sync on our examples does not cover:

  • removing empty refinements
  • ordering query params by alphabetical order

I can add them if you think it worth it.

@algobot
Copy link
Contributor

algobot commented Nov 14, 2016

By analyzing the blame information on this pull request, we identified @vvo and @bobylito to be potential reviewers

constructor() {
super();
const history = window.history;
this.state = {history, state: {...qs.parse(window.location.search.slice(1))}};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to store window.history in the state? For convenience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could totally be replaced with window.history each time yeah (like location)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok let's do that otherwise we start wondering "Why is history here?"


render() {
return <App {...this.props}
state={this.state.state}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could name it this.state.searchState?

Copy link
Contributor

@bobylito bobylito left a comment

Choose a reason for hiding this comment

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

Nice! Cool HOC for URL's :)

@vvo
Copy link
Contributor

vvo commented Nov 14, 2016

LGTM, ultimately empty refinements entries could just be removed from the state I guess?

@mthuret
Copy link
Contributor Author

mthuret commented Nov 14, 2016

Yes I was thinking the same. a new refactor to do ? :]

@vvo
Copy link
Contributor

vvo commented Nov 14, 2016

Yes I was thinking the same. a new refactor to do ? :]

Yep in another step :)

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

4 participants