-
Notifications
You must be signed in to change notification settings - Fork 156
Summary table #1335
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
Summary table #1335
Conversation
@@ -118,6 +118,7 @@ | |||
margin: 0; | |||
padding: 0; | |||
} | |||
|
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.
Sorry for the formatting changes being included in here. Let me know if this is too much a bother, and I can try to remove them.
@@ -222,7 +222,7 @@ impl SiteCtxt { | |||
.map(|bench| { | |||
( | |||
bench.name.as_str().into(), | |||
Category::from_db_representation(&bench.category).unwrap(), | |||
Category::from_db_representation(&bench.category).unwrap_or(Category::Primary), |
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.
I was getting panics locally without this. Let me know if this should be removed or if we should unwrap into Category::Secondary
.
Ignoring design for now, I think that the things that we ultimately want to show in the summary is mean and count (maybe max?), for these three axes:
Would you agree? So if I'm counting right, there should be 36 or 54 numbers shown. We should probably compress the data somehow. I like the current view, which compresses data well, vs the very large tables. On top of this, it would be later useful to show data aggregated per profile, per scenario etc., and in that case the summary will probably need to become collapsable (or collapsed by default), otherwise it will simply take too much space. We could compress the data by merging count and mean into a single "cell". Also I think that it would be ultimately nice to get rid of the All/Filtered distinction. Maybe with some button in the top part that would toggle displaying all results/turning off filters. # All
# Filtered
Primary Secondary All
Improvements: 5 (-13 %) ................. ...................
Regressions: 2 (2.5 %) ................. ...................
Total: 7 (-4 %) ................. ................... BTW before making another set of changes to the compare page, maybe it would be worthwhile to resurrect #1213 and split CSS/JS into separate files? The compare page is getting quite complex. |
Where would you put the max figure? And if we want to add more where would they go?
I'm conflicted on this. If the "all" summary is good enough then there's no need for the filter but showing a summary of the filter also allows the user to effectively "query" for this information. One could argue that we should only have a summary of everything on the page and just allow the user to query for whatever subsection they want. I'm not sure that's a good idea though.
I think so, but let's do this after this PR so we don't have to resolve conflicts. |
Going to close this. I'll probably revisit this in a new PR at some point. |
This PR replaces the summary section of the comparison page with a summary table mirroring what we show on GitHub and triage reports.
The table is not very nicely formatted, but I wanted to check that this is the direction we want to go before fiddling with CSS.