Repository navigation
Heatmaps plus - #3427
Heatmaps plus#3427fakemonster wants to merge 7 commits into
Conversation
This is naive at the moment. It only shows based on whether the first table is a set of heatmaps, and it also just inlines the radio for picking your strategy.
This is the bulk of the work here. It involves extracting these rendering settings to a less-in-the-way popover, along with some explanations of how color scaling works. The happy path experience (in my opinion) is to look at these settings at most once, but if you see them and they don't make sense, that just stinks! So the fair trade for information density is to keep it out of the way, since our defaults probably serve the vast majority of purposes, at least for now.
The previous one was more a convenience function _using_ a log, rather than actually spacing out logarithmically. A real function has to have a carveout for max counts <= 1.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I've seen the 🔥🔥🔥 screenshot but can we get one in the PR description for posterity? |
|
yeah, i'll put one in with real data |
|
@benjaminleonard we need help with this button 
|
| const scaleRank = (ranks: Map<number, number>) => | ||
| ranks.size <= 1 | ||
| ? R.constant(1) | ||
| : (count: number): number => (ranks.get(count) ?? 0) / (ranks.size - 1) |
There was a problem hiding this comment.
functional programmer in the house
| await expect(linear).toBeChecked() | ||
| await expect(ranked).not.toBeChecked() | ||
| }) | ||
|
|
There was a problem hiding this comment.
Consider adding something along these lines to check that it actually changes the chart. Could take it or leave it but it's kind of nice.
diff --git a/test/e2e/oxql.e2e.ts b/test/e2e/oxql.e2e.ts
index da6d70180e..2b82e369ed 100644
--- a/test/e2e/oxql.e2e.ts
+++ b/test/e2e/oxql.e2e.ts
@@ -117,7 +117,9 @@
await expect(page.getByRole('figure')).toHaveCount(3)
})
-test('heatmap color scale appears only for distributions', async ({ page }) => {
+test('heatmap color scale appears for distributions and updates the chart', async ({
+ page,
+}) => {
const settings = page.getByRole('button', { name: 'Chart settings' })
const colorScale = page.getByRole('group', { name: 'Heatmap color scale' })
@@ -133,9 +135,25 @@
const linear = colorScale.getByRole('radio', { name: 'Linear', exact: true })
await expect(ranked).toBeChecked()
+ const chart = page.getByRole('figure', { name: 'virtual_disk:io_latency' }).first()
+ // Cell colors have no semantic representation, so compare the canvas within this run.
+ const canvas = chart.locator('canvas')
+ await expect(canvas).toBeVisible()
+ await page.evaluate(() => document.fonts.ready)
+ const chartImage = () => canvas.evaluate((el: HTMLCanvasElement) => el.toDataURL())
+ const rankedImage = await chartImage()
+ const matchesRankedImage = async () => (await chartImage()) === rankedImage
+ await expect(chart.getByText('1', { exact: true })).toBeVisible()
+
await linear.check()
await expect(linear).toBeChecked()
await expect(ranked).not.toBeChecked()
+ await expect.poll(matchesRankedImage).toBe(false)
+ await expect(chart.getByText('0', { exact: true })).toBeVisible()
+
+ await ranked.check()
+ await expect.poll(matchesRankedImage).toBe(true)
+ await expect(chart.getByText('1', { exact: true })).toBeVisible()
})
test('results list is virtualized', async ({ page }) => {There was a problem hiding this comment.
yeah i don't mind it. the thought had occurred to me and then i was like "well idk if i want another visual test for that", but this is a nice sidestep. on that document.fonts.ready check... are you really encountering that race condition? seems vanishingly unlikely that we'd encounter that after running two queries plus the navigation
There was a problem hiding this comment.
I think Claude added that because it ran into it. Will check.
There was a problem hiding this comment.
it's a slightly odd placement, cause idk that fonts loading will cause uplot to rerender, necessarily. like it if was a problem, it should've been called before the last client action
There was a problem hiding this comment.
for now, put up a version without the font wait, and also did the same test concept for the "drop first point" option d05b1cb
This change adds color scaling controls, motivated by some in-house feedback as well as a great blog entry from Dave on the benefits of ranked coloring in particular.
This expands out to a wider introduction of a settings button for the page, because "Drop first data point" was esoteric enough; the coloring scheme, in my opinion, should not be a concern for a first-time viewer, and potentially never a concern. Plus, pushing it off to a popover gives us some wiggle room to really be descriptive for users who do want to weigh their options when picking a color scheme.
color-scaling.mp4