-
Notifications
You must be signed in to change notification settings - Fork 90
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
Plumb diff filter params through the UI #722
Conversation
Staging instance deployed by Travis CI! |
Staging instance deployed by Travis CI! |
Confirmed that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % https://diff-filter-dot-wptdashboard-staging.appspot.com/results/?products=chrome[experimental],chrome[stable]&diff&filter=C including some "0 / 0 / 0" rows, but that'll be fixed in a follow up.
.replace('product', 'before') | ||
.replace('product', 'after'); | ||
url.search = query; | ||
const productParams = url.searchParams.getAll('product'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nicer now :)
Yeah, as discussed offline, Take So, as you say, I'll be improving the client-side filtering to match the same behaviour in the UI. |
Description
Splits
path
param fromfilter
for parsing, and plumbs the value to the webcomponent.We'll need this to make the UI ignore tests that didn't run on a PR.
Also changes the
/api/diff
endpoint to allow anything that produces exactly 2 results, e.g.?product=chrome&max-count=2
Review Information
Load some filtered view, e.g.
/results?products=chrome[experimental],chrome[stable]&diff&filter=U
will only show unchanged things (kinda useless, but easy to tell it works).