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

Add a new QueryParams model #859

Merged
merged 5 commits into from
Dec 22, 2015
Merged

Add a new QueryParams model #859

merged 5 commits into from
Dec 22, 2015

Conversation

hallvors
Copy link
Contributor

This is just introducing a new QueryParams model. Nothing in this commit actually uses the new model yet, so if you want to test it you can only do so from the console:

var qp = new issueList.QueryParams();
qp.fromQueryString(location.search);
qp.setParam('label', 'firefox');
qp.setParam('stage', 'contactready');
qp.toBackendURL();

etc..

(BTW: you first have to add a SCRIPT element to the DOM that actually loads the new code..

document.body.appendChild(document.createElement('script')).src='/js/lib/models/query-params.js'

or something like that..)

@hallvors hallvors changed the title Add a new QueryParams model [not ready] Add a new QueryParams model Dec 14, 2015
@hallvors
Copy link
Contributor Author

Trying to figure out those failures.. this is weird :( Adding [not ready] to title for now.

@hallvors hallvors changed the title [not ready] Add a new QueryParams model Add a new QueryParams model Dec 14, 2015
@hallvors
Copy link
Contributor Author

Travis changed his mind when build was restarted. Weird stuff I saw locally was just a caching issue. OK to merge.

@miketaylr
Copy link
Member

Tests are passing 👍, will review once I get through e-mail backlog.

defaults:{
stage:'all',
state: 'open',
page:1,

This comment was marked as abuse.

This comment was marked as abuse.

@miketaylr
Copy link
Member

I say let's pull this in once you address the comments -- I admit I don't have the full picture of how this works in my head yet. Which means you volunteer to fix most of the bugs for /issues until I do. :p

@miketaylr
Copy link
Member

Once #860 lands, I suggest rebasing and re-running grunt to catch the eslint errors.

@miketaylr
Copy link
Member

@hallvors want to fix the eslint errors then we can pull this in?

@hallvors
Copy link
Contributor Author

eslint saved me some real debugging there 👍

Anyway, the "push" build still has this failure:

FAIL: main - Search (non-auth) - Search works (3588ms)
AssertionError: The search results show up on the page.: expected 'Issue 280: w3.org - desktop site instead of mobile site' to include 'vladvlad'

But I don't get why.. adding some code that's not even used anywhere yet shouldn't cause failures, right? What's the "push" build anyway?

@miketaylr
Copy link
Member

What's the "push" build anyway?

Travis does two builds: push (when you push) and pr (if you make a pr). Your pr build passed, but the push one failed. I've seen that particular test fail in the past, so there's likely a bug somewhere. It seems to be somewhat random... so that's fun. 💣

@miketaylr
Copy link
Member

Just retriggered the build (you can do that on Travis is you suspect weird results) and it's green now. Let's pull this one in.

miketaylr pushed a commit that referenced this pull request Dec 22, 2015
Add a new QueryParams model, #795.
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.

2 participants