Draft
Conversation
label_formatter only provides the name of the plot and the position of the point, but that makes identifying the exact data point unnecessarily complicated, when the index of the data point is available internally. This adds a new enumerated_label_formatter interface that introduces three improvements: 1. Instead of passing an empty string "" for the label when no data point is selected, we pass an Option 2. Instead of returning a String, we return an Option<String>, which allows hiding the label when we don't need it, e.g. when not hovering over a data point. 3. Instead of taking just the position and plot name, we take the position, data point index and plot name.
Author
|
There is perhaps a longer discussion to be had about the fact that the label_formatter has two purposes - providing a label in empty space, or near data points. Maybe an enum HoverPosition {
NearDataPoint {
hover_position: Point,
plot_name: &str,
data_position: Point,
data_index: usize
},
Elsewhere { hover_position: Point }
} |
Author
|
I replaced the Option usage by an enum, that makes the interface a bit more descriptive, albeit maybe also more verbose? |
I'd love to get rid of the lifetime annotations, but my Rust fu isn't good enough for that.
c7b927a to
b9d844a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
label_formatter only provides the name of the plot and the position of the point,
but that makes identifying the exact data point unnecessarily complicated,
when the index of the data point is available internally.
To change that, I wanted to add some more generic functionality, which either requires an interface break or adding a new function - I decided for the latter.
I added a new enumerated_label_formatter interface that introduces three improvements: