Limit the thickness of spans in the minimap #372
Conversation
rubenvp8510
force-pushed
the
Issue-370
branch
2 times, most recently
from
db03666
to
ff10ce8
Compare
Codecov Report
@@ Coverage Diff @@
## master #372 +/- ##
==========================================
+ Coverage 88.17% 88.17% +<.01%
==========================================
Files 157 157
Lines 3493 3494 +1
Branches 798 798
==========================================
+ Hits 3080 3081 +1
Misses 376 376
Partials 37 37
Continue to review full report at Codecov.
|
| @@ -36,7 +37,9 @@ export default function renderIntoCanvas( | |||
| canvas.width = cWidth; | |||
| // eslint-disable-next-line no-param-reassign | |||
| canvas.height = cHeight; | |||
| const itemHeight = Math.max(MIN_ITEM_HEIGHT, cHeight / items.length); | |||
| const computedItemHeight = Math.max(MIN_ITEM_HEIGHT, cHeight / items.length); | |||
| const itemHeight = computedItemHeight > MAX_ITEM_HEIGHT ? MAX_ITEM_HEIGHT : computedItemHeight; | |||
There was a problem hiding this comment.
Why mix ?: and max?
w = Math.min(MAX, Math.max(MIN, expr))
There was a problem hiding this comment.
I just wanted to be more explicit, but I think using math.min and math.max is more elegant. I'll change.
Signed-off-by: Ruben Vargas <[email protected]>
There was a problem hiding this comment.
Another alternative implementation could be to limit the height of the minimap canvas to min(currentHeight, spans*spanHeight). Then the existing span height logic would remain the same. But I don't know how the minimap would look aesthetically.
Could you add a screenshot of how it now looks with a small trace (a couple of spans)?
| const width = valueWidth / totalValueWidth * getCanvasWidth(); | ||
| const x = valueOffset / totalValueWidth * getCanvasWidth(); | ||
| const y = height * i; | ||
| const y = cHeight / items.length * i; |
There was a problem hiding this comment.
Can we have the same min/max logic in the test? It's easier to read.
There was a problem hiding this comment.
Actually the test has the min/max logic,
There was a problem hiding this comment.
I'll attach screenshots with small trace. But I think if we limit the height of the minimap for this case we will end with a really small minimap (not too much aesthetic IMHO) but I'll attach both screenshots.
|
I attached 3 possible implementations here.
|
|
the last two screenshots look wrong - if we reduce the height of the minimap, we should still calculate it as MIN(max_allowed_height, default_span_height * span_count), where default_span_height is the same as in the main timeline view. |
|
Ha! I did the calculations using MIN(max_allowed_height, default_span_height * span_count) but I used the MIN_ITEM_HEIGHT as a default_span_height, that explains why it looks bad. If I understand correctly default_span_height should be the height of the span in the main timeline. Ok I'll update it. |
|
Thanks for review! |
|
that's what I was suggesting, but now that I see it, it doesn't look particularly great (probably because there's no whitespace between spans). @tiffon what are your creative suggestions? |
|
1 looks the great, IMO. Is that the one currently set to be merged? |
|
Yes, option 1 is implemented in this PR. |
Limit the thickness of spans in the minimap Signed-off-by: vvvprabhakar <[email protected]>
Which problem is this PR solving?
Short description of the changes