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

Start a new top table report type #545

Merged
merged 18 commits into from
Feb 21, 2022
Merged

Start a new top table report type #545

merged 18 commits into from
Feb 21, 2022

Conversation

metalmatze
Copy link
Member

@metalmatze metalmatze commented Jan 12, 2022

We need to follow up in a couple of places:

  • Make sure to take all lines into account (inline functions)
  • Merge nodes/samples by their function name if location addresses match (in one example lines went from ~4600 => ~375)

Other than that we need to obviously build the UI parts :)

@metalmatze metalmatze mentioned this pull request Jan 12, 2022
metalmatze and others added 3 commits January 13, 2022 12:38
@brancz
Copy link
Member

brancz commented Jan 27, 2022

Is this ready for review?

@metalmatze
Copy link
Member Author

We still have a few TODOs left to do from the backend and UI side. If you want you can obviously give feedback already! ☺️

@brancz
Copy link
Member

brancz commented Jan 28, 2022

I'll hold off until you deem it ready :) let me know when that's the case, looking forward to this!

This adds side by side view for the top table and also sorting by table headers
@brancz
Copy link
Member

brancz commented Feb 10, 2022

What's the status on this?

@metalmatze
Copy link
Member Author

I need to finish the aggregation work. I wanted to do this in a stream but didn't end up streaming. I'll finish it regardless then.

@yomete
Copy link
Contributor

yomete commented Feb 14, 2022

I just pushed my latest UI changes to make the table look nice. it has all the agreed features e.g sorting. @metalmatze it'd be nice to also receive the units from the backend so we can use that to format the numbers in the table.

Lastly, did we say we wanted to have additional columns for Flat% and Cum%? Let me know so i can add the columns and what their keys will be :)

@metalmatze
Copy link
Member Author

Very nice!
I'll work on the backend side to finish this up.
Let's make this PR work as is and then follow up with another PR to add the percentages if we think it's better with than without?

@yomete
Copy link
Contributor

yomete commented Feb 15, 2022

Very nice! I'll work on the backend side to finish this up. Let's make this PR work as is and then follow up with another PR to add the percentages if we think it's better with than without?

sounds good!

@metalmatze
Copy link
Member Author

Another round of reviews would be perfect now! ☺️

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

Amazing!! lgtm 🎉

@brancz brancz merged commit c3218b4 into main Feb 21, 2022
@brancz brancz deleted the top-table branch February 21, 2022 12:59
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