Enhance Distribution Diagramm

Kr0nox commented 3 months ago

This PR improves the distribution diagram by

adding a legend to it
Giving options for the number of buckets

Addresses parts of #1606

Kr0nox commented 3 months ago

The animation when swapping to a different distribution granularity was confusing at the beginning, and still is when trying to compare two different granularities. I assume this is nothing we implemented on purpose and is rather part of an API, right?

This is the default, but can be disabled (would also make the e2e tests faster)

At finer granularities, the numbers disappear. I notice that I swap back to one where I can see the numbers. This is especially the case when the lower percentage submissions are so many, that the bars for the higher percentage one are barely visible. If we can show the numbers for more or all granularities, this would be better.

This was done on purpose, since numbers would overlap and not be readable, but I can disable them for every second bar or something depending on the granularity. Your thoughts on that @tsaglam?

Generally, I think we can stick to 4 or so granularities, e.g. 10, 20, 25, and 50.

I would suggest 10, 25,50, 100 since 20 and 25 are pretty close together

tsaglam commented 3 months ago

This is the default, but can be disabled (would also make the e2e tests faster)

I think I would prefer that.

This was done on purpose, since numbers would overlap and not be readable, but I can disable them for every second bar or something depending on the granularity.

Can we scale the numbers, or is that unnecessarily complicated? Every second is not that helpful, as there is not necessarily a relation between the number of comparisons of neighboring buckets

I would suggest 10, 25,50, 100 since 20 and 25 are pretty close together

Sounds good, we can always adjust if we feel we need different granularities.

Kr0nox commented 3 months ago

Removing the animations made the e2e test for the distribution diagram very flaky, especially on WebKit. I could ever search for a solution, but that might delay the development of this PR. I could also disable the tests for now and re-enable them when a solution was found

tsaglam commented 3 months ago

Then I would say we keep the animations for now, and turn this into an issue. We can disable animations whenever we find a suitable solution for the tests.

tsaglam commented 3 months ago

Ready to merge, @Kr0nox?

Kr0nox commented 3 months ago

yes. should I add the animation issue to a new one, or the already existing one about the distribution diagram?

tsaglam commented 3 months ago