Closed L-Andrade closed 5 months ago
Hey @patrickmichalik, thanks for the feedback 😄
Hello, and thanks for the PR! When
LabelPosition.Bottom
is applied,DefaultCartesianMarker
should request a bottom inset to ensure that there’s enough room for the label. There may not be a bottom axis, or the height of the label may exceed that of the bottom axis. Could you updategetInsets
(line 189) accordingly? This modification won’t change the end result in instances where there’s already sufficient room.
Ah, missed that! Good point, wouldn't look good without that. I've tested it and it works nicely after this commit: f56e3fdfa8f0a1249e94b38c395944566cb85b8d
Here's a couple screenshots, I've added some background colors so we can easily identify the chart's size | Bottom | Top |
---|---|---|
It took me a while to notice that Marker.kt
's marker had an override for getInsets
. Since the code was basically the same as the getInsets
except for the addition of the shadow clipping, I changed it to call the super
's getInsets
and to add the clipping after, which makes it easier to test other LabelPosition
s' behaviour.
Changing the code to use when
should also make it easier in the future if there's a need for other label positions, as they will require it to be exhaustive (for example, if there's a need to add BelowPoint
in the future - it won't compile until we update it in all these places now)
Let me know if you'd prefer the previous implementation there (in Marker.kt
), since it is just sample code. I'll be happy to roll it back if needed.
Thanks @patrickmichalik!
For completeness, I wanted to note that I missed a minor issue here. Whatever the condition for the bottom shadow inset being added may be, its size should be different than that of the top shadow inset. This is because the shadow is always shifted downward by 2 dp. I erroneously concluded that two different sizes would be required only once the out-of-scope change mentioned is made—perhaps because I flipped the shadow along with the label background in my head. I should’ve suggested defining two sizes, at which point the logic might as well have been updated to always add both shadow insets, even if this isn’t strictly related.
I hadn’t yet merged the PR when I realized this, but it’s a small thing, and it’s my mistake for overlooking it in the review. I apologize. I’ll take care of it when removing the conditional behavior.
@patrickmichalik Sorry for missing it too - makes sense. Let me know if you need anything from my side!
No worries, @L-Andrade! Such details sometimes slip through the cracks. I’ve fixed the issue here. Thanks again!
Adds
Bottom
as aLabelPosition
, which positions at the bottom of the graph. Basically the inverse ofTop
. Solves https://github.com/patrykandpatrick/vico/discussions/701