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

Add Bottom to LabelPosition for Cartesian Charts #721

Merged

Conversation

L-Andrade
Copy link
Contributor

Adds Bottom as a LabelPosition, which positions at the bottom of the graph. Basically the inverse of Top.
Solves #701

Bottom Top
Screenshot 2024-05-22 at 14 09 20 Screenshot 2024-05-22 at 14 24 56

Copy link
Member

@patrickmichalik patrickmichalik left a comment

Choose a reason for hiding this comment

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

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 update getInsets (line 189) accordingly? This modification won’t change the end result in instances where there’s already sufficient room.

@L-Andrade
Copy link
Contributor Author

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 update getInsets (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: f56e3fd

Here's a couple screenshots, I've added some background colors so we can easily identify the chart's size

Bottom Top
Screenshot 2024-05-23 at 16 38 03 Screenshot 2024-05-23 at 16 38 35

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 LabelPositions' 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.

Copy link
Member

@patrickmichalik patrickmichalik left a comment

Choose a reason for hiding this comment

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

Thanks, @L-Andrade! The changes that you mention look good. I have three minor suggestions.

Copy link
Member

@patrickmichalik patrickmichalik left a comment

Choose a reason for hiding this comment

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

Cheers! @Gowsky is on a business trip at the moment, but he told me before leaving that he didn’t have any further suggestions, so we’re ready to go.

@patrickmichalik patrickmichalik merged commit 9f21a5a into patrykandpatrick:master May 27, 2024
3 checks passed
@L-Andrade L-Andrade deleted the feature/labelposition-bottom branch May 27, 2024 13:38
@L-Andrade
Copy link
Contributor Author

Thanks @patrickmichalik!

@patrickmichalik
Copy link
Member

patrickmichalik commented May 27, 2024

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.

@L-Andrade
Copy link
Contributor Author

@patrickmichalik Sorry for missing it too - makes sense. Let me know if you need anything from my side!

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