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

ui (histograms): Add native histogram chart to Table view #13658

Merged
merged 12 commits into from Feb 29, 2024

Conversation

Maniktherana
Copy link
Contributor

addresses #11269

This PR adds a better rendered view of native histograms in the table view. The histogram can be toggled between using a linear or exponential scale on the x-axis.

Screen.Recording.2024-02-28.at.10.32.30.AM.mov

Signed-off-by: Manik Rana <manikrana54@gmail.com>
Signed-off-by: Manik Rana <manikrana54@gmail.com>
Signed-off-by: Manik Rana <manikrana54@gmail.com>
@Maniktherana
Copy link
Contributor Author

@juliusv here's what I have so far. I need to add some tests and there may be room to improve how we display the total count and sum but it felt off when adding those in the same table as range + count

Signed-off-by: Manik Rana <manikrana54@gmail.com>
Signed-off-by: Manik Rana <manikrana54@gmail.com>
Signed-off-by: Manik Rana <manikrana54@gmail.com>
@Maniktherana Maniktherana marked this pull request as draft February 28, 2024 05:24
Signed-off-by: Manik Rana <manikrana54@gmail.com>
@Maniktherana
Copy link
Contributor Author

Maniktherana commented Feb 28, 2024

Hmm, it seems the <HistogramChart /> component isn't rendering in tests. Not sure why

Signed-off-by: Manik Rana <manikrana54@gmail.com>
Signed-off-by: Manik Rana <manikrana54@gmail.com>
Copy link
Member

@juliusv juliusv left a comment

Choose a reason for hiding this comment

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

Nice, I like it so far, thanks! Yeah, now seeing the count and sum outside of the bucket table, have to actually say that I prefer that too.

@@ -122,6 +122,128 @@ input[type='checkbox']:checked + label {
color: $checked-checkbox-color;
}

.summary-wrapper {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this .histogram-summary-wrapper or something like that because otherwise it sounds very generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

width: 100%;
bottom: 0;
// background-color: #9090ff;
// border: 1px solid #aaaaff;
Copy link
Member

Choose a reason for hiding this comment

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

Are these commented-out lines still needed, or should they be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not needed

Signed-off-by: Manik Rana <manikrana54@gmail.com>
@Maniktherana
Copy link
Contributor Author

@juliusv this is pretty much done I'd say, only thing is I'm having trouble understanding why the UI test is failing.

Test suite failed to run

    The target 'bucket-0-0-500' could not be identified in the dom, tip: check spelling

      130 |       useLocalTime: false,
      131 |     };
    > 132 |     const dataTable = mount(<DataTable {...dataTableProps} />);
          |                            ^
      133 |
      134 |     it('renders a table', () => {
      135 |       const table = dataTable.find(Table);

A little bit unsure how to proceed. I could just forgo testing <HistogramChart /> in Datatable.test.tsx and just keep unit tests for <HistogramChart />

@juliusv
Copy link
Member

juliusv commented Feb 29, 2024

@Maniktherana Strange! Yes, I think it's ok to skip testing it for now. We shouldn't be spending too much on figuring out weirdness in Enzyme tests that we will have to throw away anyway with the new Mantine-based UI and React 18.

Signed-off-by: Manik Rana <manikrana54@gmail.com>
@juliusv
Copy link
Member

juliusv commented Feb 29, 2024

👍 Do you want to press the "Ready for review" button to move the PR out of draft status? Then I'll merge it.

@juliusv juliusv marked this pull request as ready for review February 29, 2024 13:20
@juliusv
Copy link
Member

juliusv commented Feb 29, 2024

Meh actually I'll just do it myself :)

@juliusv juliusv merged commit ab9b770 into prometheus:main Feb 29, 2024
24 checks passed
@Maniktherana
Copy link
Contributor Author

@juliusv Awesome, thanks! Ig there's some ancillary issues I can create like testing + optimizing histogram rendering

@juliusv
Copy link
Member

juliusv commented Feb 29, 2024

@Maniktherana Thanks as well! Yeah I think functional / performance improvements would be more useful at the moment, with the current set of tests going away soon. I think in the new UI I will hide all the graph page settings in a collapsible section, which would make it easier to add even more settings in there - e.g. to configure how histograms should be displayed by default in the table.

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.

None yet

2 participants