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

Query params url#2547 #2550

Merged

Conversation

Projects
None yet
3 participants
@hjhimanshu01
Copy link
Contributor

commented Mar 4, 2019

Opening the Pull Request

  • Note the issue that the PR addresses. Include Fixes #2547
  • Added query parameters to the URL address ( search_term , search_type , article_quality , min_views) for sharing the URL
    *On refreshing the page , URL is restored to the original one without any search parameters
    *Added screenshot
    screenshot from 2019-03-04 00-23-55
@ragesoss

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

Cool!
"On refreshing the page , URL is restored to the original one without any search parameters" - this is not the desired behavior. The main use case is the copy the URL with the parameters so that it can be shared with someone else and the same search can be replicated.

@hjhimanshu01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

I do understand that though if the user refreshes the page (even by mistake) , old query params would persist in the URL while the article searched would be none , leading to inaccurate results if URL is shared at that point , just an edge case , shall I still remove it ?

@ragesoss

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I'm not sure I understand the problem you're describing.

Here's what I think the ideal behavior would be:

  • URL does not change until I click 'Submit', at which point it updates the URL to include the params
  • Refreshing the page with those params in the URL results in the search term and other params automatically updating the initial state of the refreshed page, so that clicking 'Submit' will re-run the same search.
  • Any changes to the search that I make will get updated in the URL when I click 'Submit' again.

hjhimanshu01 added some commits Mar 5, 2019

@hjhimanshu01

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@ragesoss ,done ! added all the functionalities ideally required !

@ragesoss

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

Aside from the pushState issue, this works very nicely!

I'm finding the control flow pretty confusing, though. The trip from getQueryVariable, to (async) this.updateFields, this.props.updateFields, and buildUrl, and then back to this.updateFields for user interactions takes a while to think through, and I'm still unclear about what the async, await is doing. If you can think of any ways to refactor this for clarity, please do. (Maybe hooks?)

@bwreid can we get your feedback on this?

Show resolved Hide resolved db/schema.rb Outdated

@bwreid bwreid self-requested a review Mar 5, 2019

@bwreid
Copy link
Contributor

left a comment

@hjhimanshu01 Sorry it took me a bit to get to this! Thanks for the great Pull Request. :)

👍 to using .replaceState() instead.

I think some of the trickiness here with how the data is getting passed around is unfortunately inherent to how different parts of this component need to use each of those smaller functions. I suggested a change to the getQueryVariable() method which might help. But, feel free to push back if you think it's more confusing or if you want something explained. :)

@bwreid
Copy link
Contributor

left a comment

This is looking really great! Just added a few more comments, mostly stylistic stuff. Thanks for working on this!

return this.props.updateFields(key, value);
const update_field = this.props.updateFields(key, value);
Promise.resolve(update_field).then(() => {
if (this.props.search_term.length !== 0) { this.buildURL(); }

This comment has been minimized.

Copy link
@bwreid

bwreid Mar 8, 2019

Contributor

Let's fix this indentation. 👍

This comment has been minimized.

Copy link
@hjhimanshu01

hjhimanshu01 Mar 8, 2019

Author Contributor

Okay!

queryStringUrl += `?search_term=${this.props.search_term}`;
params_array.map((param) => {
return queryStringUrl += `&${param}=${this.props[param]}`;
});

This comment has been minimized.

Copy link
@bwreid

bwreid Mar 8, 2019

Contributor

In this case, you're really just using .map() as a .forEach(). You can change it to a .forEach() or we could use .reduce(). I think the following would work:

queryStringUrl = params_array.reduce((url, param) => {
  return url + `&${param}=${this.props[param]}`;
}, queryStringUrl);

I don't really have a preference! In fact, I think @ragesoss might like the .forEach() better. :)

This comment has been minimized.

Copy link
@ragesoss

ragesoss Mar 8, 2019

Member

Heh. .reduce() is fine here. But I probably would have used .forEach() instead if I'd written it myself, yeah.

This comment has been minimized.

Copy link
@hjhimanshu01

hjhimanshu01 Mar 8, 2019

Author Contributor

Let's go with .forEach() ! :)

},

toggleFilter() {
return this.setState({
showFilters: !this.state.showFilters
});
},

buildURL() {
let queryStringUrl = window.location.href.split('?')[0];

This comment has been minimized.

Copy link
@bwreid

bwreid Mar 8, 2019

Contributor

Should we use window.location.search here as well?

This comment has been minimized.

Copy link
@hjhimanshu01

hjhimanshu01 Mar 8, 2019

Author Contributor

window.location.search gives results after the "?" , it's better to use window.location.href as it gives the full address , that's why didn't changed it.

Reference : https://webplatform.github.io/docs/apis/location/search/

This comment has been minimized.

Copy link
@bwreid

bwreid Mar 8, 2019

Contributor

Ah, great! :)

This comment has been minimized.

Copy link
@hjhimanshu01

hjhimanshu01 Mar 8, 2019

Author Contributor

Done! :D

hjhimanshu01 added some commits Mar 8, 2019

@bwreid bwreid merged commit 709010b into WikiEducationFoundation:master Mar 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.