-
Notifications
You must be signed in to change notification settings - Fork 89
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
Remove Edge as a default product, but add breadcrumb link #2431
base: main
Are you sure you want to change the base?
Conversation
This flag was used to fetch edge runs back when they were not tagged with the experimental label. It is no longer useful, and is also misleading (the name implies it might remove Edge from the default query, which it does not do). See #2431 (comment) This change also makes util/populate_dev_data.go closer to what is shipped on wpt.fyi - the flags are updated and a new Edge experimental test run is added for the local testing sha. The data for that run is a copy of the Edge stable data for the same sha. Finally, the `webdriver/` README.md file is extensively updated with more description and a lot of debugging tips I've discovered over the last day!
After a lot of discussion, we have decided to remove Edge as a default product from wpt.fyi due to overlap in browser engine between Chrome and Edge. In a perfect world we would invest in per-user default products, but there is no resourcing for that currently. To try to minimize the disruption, we add a breadcrumb link back to easily let users get Edge back on the page. See #1519
53d3853
to
007e04f
Compare
e1e0544
to
afa9e81
Compare
afa9e81
to
003914b
Compare
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 on the code. I will leave @foolip for the final stamp
@@ -386,6 +390,21 @@ class WPTApp extends PathInfo(WPTFlags(TestRunsUIBase)) { | |||
return true; | |||
} | |||
|
|||
showAddEdgeBackLink(queryParams) { |
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.
Also need to check if queryParams.run_id
is true. If we are querying by run_id
, we cannot simply add Edge to the UI
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.
Another case where we can't add Edge back is the links for PR checks, such as https://wpt.fyi/?sha=ec5f766aec&label=pr_head. There is no Edge run to add back in that case.
The ideal behavior would be to detect the cases where the runs we are showing are inferred from the default product set, and where adding Edge to that default would still result in some runs to show. They might not be for the same commit in the case of aligned runs, but an additional run from Edge should appear.
Implementing this would presumably requiring using /api/runs
a few times, so are there simpler approximations of this that get it right in most cases?
When loading https://smcgruer-remove-edge-d-dot-wptdashboard-staging.uk.r.appspot.com/ I still see Edge there. Are staging deployments not working? |
Looks like its not deployed properly. Let me close and open it |
@foolip Should be working now! |
What's the status here? |
@KyleJu @past and I have talked about what to do about "default product set" when we add a front page. In short, there will be a widget on the front page to get to results, and that widget will have Chrome, Firefox and Safari selected by default, but can be changed with a few clicks. All URLs will specify the products to show, or redirect to URLs that do. I think we should redirect to the current default for existing product-less URLs. A side effect of this will be that there won't be any short URLs that show what you (probably) want. |
After a lot of discussion, we have decided to remove Edge as a default
product from wpt.fyi due to overlap in browser engine between Chrome and
Edge. In a perfect world we would invest in per-user default products,
but there is no resourcing for that currently.
To try to minimize the disruption, we add a breadcrumb link back to
easily let users get Edge back on the page.
See #1519