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

Clicking on label from /issues page does not update URL params #1074

Closed
deepthivenkat opened this issue May 31, 2016 · 9 comments
Closed

Clicking on label from /issues page does not update URL params #1074

deepthivenkat opened this issue May 31, 2016 · 9 comments
Assignees

Comments

@deepthivenkat
Copy link
Member

deepthivenkat commented May 31, 2016

Steps to reproduce:

Go to issues page. Click on a label for any of the issues. Eg : firefox-mobile

Expected Result:

In url: https://webcompat.com/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc&q=label%3Abrowser-firefox-mobile
In search box : label:browser-firefox-mobile

Actual Result:

In url: https://webcompat.com/issues?page=1&per_page=50&state=open&stage=all&sort=created&direction=desc [The url is not the same as expected]
In search box: label:browser-firefox-mobile

@deepthivenkat
Copy link
Member Author

deepthivenkat commented May 31, 2016

In the file issue-list.html

I changed :

<a href="/issues?labels=<%= label.remoteName %>"

to

<a href="/issues?q=label:<%= label.remoteName %>"

When I do the above changes, The URL is getting updated with the previously selected label and not the current label.(Check attachment)

animation

I tried making multiple changes.

i) I edited
issue-list.js

LabelSearch method and included the following lines:

this.updateModelParams('q=' + labelFilter, {removeQ: false});
      // clear any filters that have been set.
      issueList.events.trigger('filter:clear', {removeQ: false});

after the line issueList.events.trigger('issues:update', {query: labelFilter});

ii) I tried using history.pushstate straight away to modify the URL when a label is clicked. The above two didn't work.

Any ideas here?

@miketaylr
Copy link
Member

@deepthivenkat so Scenario 2 and 3 are bugs, right? Can we get individual issues filed for those?

It's confusing to discuss multiple bugs in a single issue.

A good template for filing bugs is like this:

[descriptive bug title]

Steps to reproduce:
1) Go here
2) Click here
3) etc.

Expected Results:
Thing happens.

Actual Results:
Other thing happens

Scenario 2's bug title might be: "Clicking on label from /issues page does not update URL params"
Scenario 3 might be: "Right-clicking on issue label to open in new tab does not have correct URL param state or search term in input box"

Thanks!

@deepthivenkat deepthivenkat changed the title Clicking on a label in the issue list make no change in the URL Clicking on label from /issues page does not update URL params May 31, 2016
@hallvors
Copy link
Contributor

@deepthivenkat Our search/filtering UI and URL state here leave some things to be desired - see #795 and friends. I've started working on fixes in this area, among them is a chunk of code taken in in #859 which is not actually used anywhere but (I hope) should help solve this bug and much of the related stuff. (PRs 835 and 846 are unsuccessful attempts at fixing everything - we should take this in smaller steps. However, if the next small step is you fixing the URL issue here using - if it works for you - code from #859 that would be sweet).

@deepthivenkat
Copy link
Member Author

@miketaylr I have edited this issue to contain changes relevant to

Clicking on label from /issues page does not update URL parameters

Opening a separate issue for other bugs.

Thanks!

@miketaylr
Copy link
Member

miketaylr commented May 31, 2016

When I do the above changes, The URL is getting updated with the previously selected label and not the current label.(Check attachment)

The way clicking on labels in issue-list works is like so:

There's an event listener:
https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issue-list.js#L276

Which calls the labelSearch method:
https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issue-list.js#L388

The actual markup for the label is <a href="/issues?labels=browser-firefox" class="wc-Labels" data-remotename="browser-firefox" title="Labels : firefox">firefox</a>

I think the ?labels=browser-firefox stuff isn't actually used -- it's from before when we made some updates around labels (and possibly forgot to update here) IIRC, the idea was to support right-click -> open in new tab with that param. So your proposed change to ?q=label:<%= label.remoteName %>" is a good one.

Handling the params update when we trigger update:issues (which ends up calling updateIssues) is here: https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/issue-list.js#L396.

Warning, it can get pretty complicated here. But I would start trying to understand what the current state of the model params is (this.issues.params) and figuring out why the {query: labelFilter} data of the update:issues event isn't getting propagated to the URL bar.

@hallvors
Copy link
Contributor

It took me a long time to understand why some code was using q=label:foo - type syntax and other code was using label=foo syntax. But everything I managed to figure out (hint: it has to do with the GitHub backends) is documented in the comments in the file https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/models/query-params.js . Hope that's useful at some point!

@deepthivenkat
Copy link
Member Author

Thanks @hallvors

@deepthivenkat
Copy link
Member Author

#1078

@deepthivenkat
Copy link
Member Author

deepthivenkat commented Jun 1, 2016

👍 Will add detailed description in the PR! Yes. I will write Intern tests. Opening a separate issue for that! Thanks @miketaylr .

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

No branches or pull requests

4 participants