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

Update compare page UI #1612

Merged
merged 10 commits into from
Jun 9, 2023
Merged

Update compare page UI #1612

merged 10 commits into from
Jun 9, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jun 7, 2023

This PR modifies the UI of the compare page, with several goals:

  1. Prepare the page for the inclusion of runtime benchmarks by introducing tabs, combined with a quick summary. This allows us to see bootstrap results without scrolling down a lot.
  2. Remove unnecessary padding and spaces and cramp stuff closer together, so that more of the compare table can be seen outright .
  3. Make it look slightly nicer on mobiles.

The commits are mostly atomic and independent, so we can land only some of the changes, if they are too controversial.
More changes could be made, e.g. to put some things that belong together more closer together (such as the quick links with the "Do another comparison"), and in general the page could definitely use some UX love, but for now I think that this is good enough.

Before

image

After

image

Video (outdated design)
perf.mp4

@Kobzol Kobzol requested review from lqd and nnethercote June 7, 2023 14:52
@nnethercote
Copy link
Contributor

Initial comments just based on the picture and video (I haven't really looked at the code).

  • Strong preference: in the "Compile time" tab box you have three columns, "min", "max", "avg". Please make this look like the summary within the tab, which has two columns "Range" and "Mean" for this data. Also, the colour in the tab box should probably just be black, to match the colour used in the third line of the summary within the tab.
  • Weak preference: putting the summary box to the right of the "Filters" section is a bit ugly, and doesn't seem like it saves much vertical space. Move it back?
  • Question: does the "Compile time" tab box update when you change the selections in "Filters"?

Otherwise, seems reasonable. Having tabs is good.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 8, 2023

Strong preference: in the "Compile time" tab box you have three columns, "min", "max", "avg". Please make this look like the summary within the tab, which has two columns "Range" and "Mean" for this data. Also, the colour in the tab box should probably just be black, to match the colour used in the third line of the summary within the tab.

Good point, I originally had a different layout for these three values, where it couldn't use the same style as the original summary table, but now it's a table anyway, so it should be unified.

Regarding the colors, I'm a bit hesitant here. The colours in the current summary signify "improvement" or "regression", regardless of the value of the number (although all improvements are negative and all regressions positive, of course). In the tabs, the colour distinguishes positive and negative numbers, which is slightly inconsistent, as you have noticed. However, I feel like including colors in the tabs (which should serve as a first-look summary of the comparison, see below), is helpful.

One way out of this could be to change the current summary tables like this:
image

and/or add ❌,✅ to the tab table.

Weak preference: putting the summary box to the right of the "Filters" section is a bit ugly, and doesn't seem like it saves much vertical space. Move it back?

I know, it's so ugly 😭 But it saves some vertical space 😆 Probably it's not worth it though.

Question: does the "Compile time" tab box update when you change the selections in "Filters"?

No (by design). The tabs should (apart from navigation) also serve as a super quick summary for evaluating the results of the comparison at a first glance. They contain results from all relevant measurements and are not updated when you change the filters.

@lqd
Copy link
Member

lqd commented Jun 8, 2023

If you want to save vertical space, you could have smaller buttons to change tabs, a variety of heights, up to link-heights that could be positioned on the same line as the quick links.

@nnethercote
Copy link
Contributor

Using green for all improvements and red for all regressions looks fine to me.

@Kobzol
Copy link
Contributor Author

Kobzol commented Jun 9, 2023

Changed tab to align it better with the existing summary, changed colors in summary table(s) and right-aligned the columns.

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only skimmed the code changes. The UI changes look good.

@Kobzol Kobzol merged commit 3ba1b59 into master Jun 9, 2023
@Kobzol Kobzol deleted the compare-page-ui branch June 9, 2023 09:58
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.

3 participants