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.

Sign up for GitHub

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

Jump to bottom

feat: persistent search keyword in URI as permalink #53

Merged
merged 4 commits into from
Oct 15, 2018
Merged

feat: persistent search keyword in URI as permalink #53

merged 4 commits into from
Oct 15, 2018

Conversation

iori-yja
Copy link
Contributor

This is intended to reflect search query in the URI so that people can exchange a link to the graph with highlighting some parts of it.

  • On the invocation of "search" button, the query-string is changed by history.push() (pushState() in html5)
  • With "clear" button, the query-string in the URI is cleared, as well as the result and input box.
  • With a change of URI (such as navigation transition or direct typing), search result and input box respond to the new query.

@spiermar spiermar self-requested a review July 26, 2018 20:07
@spiermar spiermar self-assigned this Jul 26, 2018
@spiermar spiermar added the review Tagged for review. label Jul 26, 2018
@spiermar spiermar added this to the 0.2 milestone Jul 26, 2018
@spiermar spiermar changed the title Persistent search keyword in URI as permalink feat: persistent search keyword in URI as permalink Sep 28, 2018
@spiermar spiermar removed the review Tagged for review. label Oct 8, 2018
spiermar
spiermar approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@spiermar spiermar left a comment

Choose a reason for hiding this comment

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

LGTM. Merging it. Just need to remove the else statement and open an issue to track the overall deep linking feature.

src/components/FlameGraph/FlameGraph.jsx
if (sq) {
this.setState({searchTerm: sq});
this.state.chart.search(sq);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for else.

@spiermar spiermar merged commit 6aeec55 into Netflix:master Oct 15, 2018
@spiermar spiermar mentioned this pull request Oct 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiermar spiermar spiermar approved these changes

Assignees

@spiermar spiermar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@iori-yja @spiermar