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

[issue 1630] Triggering search via URL #1657

Closed
wants to merge 6 commits into from

Conversation

DukeManh
Copy link
Contributor

@DukeManh DukeManh commented Feb 5, 2021

Issue This PR Addresses

Fixes #1630

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Thanks for starting on this. A few things:

  • update the title of your PR so it addresses both the issue number and also provides a sentence about what it fixes. See other PRs for examples
  • the logic for managing this state needs to be fixed. In the original Gatsby version we do this:
  // We synchronize the `text` and `filter` values to the URL's query string
  // via `textParam` and `filterParam`. The <SearchBar> UI uses our internal
  // state values, and the <SearchResults> only update on page load or when
  // the user submits the form.
  const [textParam = '', setTextParam] = useQueryParam('text', StringParam);
  const [filterParam = 'post', setFilterParam] = useQueryParam('filter', StringParam);

  // We manage the state of `text` and `filter` internally, and update URL on
  // form submit only.  These are used in the <SearchBar>, and the user can change them.
  const [text, setText] = useState(textParam);
  const [filter, setFilter] = useState(filterParam);

  // Form was submitted, so go ahead and sync to URL, (re)triggering search.
  function onSubmitHandler(event) {
    event.preventDefault();
    setTextParam(text);
    setFilterParam(filter);
  }

The "state" of the filter/text params doesn't need to be in React, it's in the URL. We just need to read it out, and use it as a local variable. So we should be pulling this from router.query and syncing it back when we do router.push(...) the new URL (which will update the state on the URL).

@DukeManh
Copy link
Contributor Author

DukeManh commented Feb 5, 2021

Thank you. I just wasn't sure to use useQueryParam hook or to manage params in React. I will update this so that we follow Gatsby implementation.

@humphd
Copy link
Contributor

humphd commented Feb 5, 2021

We have to use the next router to do this, so it will be different from useQueryParam

@chrispinkney chrispinkney linked an issue Feb 6, 2021 that may be closed by this pull request
@chrispinkney chrispinkney added this to the 1.7 Release milestone Feb 6, 2021
@chrispinkney chrispinkney added this to In progress in Nextjs via automation Feb 6, 2021
@chrispinkney chrispinkney added the type: bug Something isn't working label Feb 6, 2021
@chrispinkney
Copy link
Contributor

chrispinkney commented Feb 6, 2021

Welcome!

Please also add a description and add which issue this fixes under "Issue This PR Addresses" (you can simply say Fixes #1630) 😄

@huyxgit huyxgit changed the title Issue 1630 [issue 1630] Triggering search via URL Feb 7, 2021
@DukeManh
Copy link
Contributor Author

DukeManh commented Feb 8, 2021

@humphd I have followed your request, searching through URL is now working.

Nextjs automation moved this from In progress to Review in progress Feb 9, 2021
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Awesome. Testing this with https://deploy-preview-1657--telescope-next.netlify.app/search?text=react&filter=post I notice that it works if you do it in the app, but try copy/pasting that URL into a new window, and the initial values being passed down to the input box on props aren't being set on first render.

HyperTHD
HyperTHD previously approved these changes Feb 13, 2021
Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

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

@DukeManh You're going to need to rebase this. Please do so ASAP so we can get this merged in.

Working Search through URLs

@DukeManh DukeManh closed this Feb 14, 2021
@DukeManh DukeManh deleted the issue-1630 branch February 14, 2021 04:39
Nextjs automation moved this from Review in progress to Done Feb 14, 2021
@DukeManh DukeManh restored the issue-1630 branch February 14, 2021 04:40
@DukeManh DukeManh deleted the issue-1630 branch March 24, 2021 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Nextjs
  
Done
Development

Successfully merging this pull request may close these issues.

Triggering search through URL doesn't work.
5 participants