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 data point labels #161

Merged
merged 3 commits into from
Jun 3, 2023
Merged

Add data point labels #161

merged 3 commits into from
Jun 3, 2023

Conversation

rdnetto
Copy link
Contributor

@rdnetto rdnetto commented May 9, 2017

This PR adds the ability to annotate scatter plots and bar graphs with labels. The supported options mirror the arguments of drawTextR, with some additional support for controlling position relative to the point/bar.

@timbod7
Copy link
Owner

timbod7 commented May 11, 2017

Thanks for this PR.

Were you aware of the Graphics.Rendering.Chart.Plot.Annotation type? I would have expected that it would meet your needs for adding textual annotations to point and line plots.

The bar plots change looks valuable. However I would expect that the anchor values would be the same for each label, and hence could be passed once, rather than on each label. Also, Requiring the two parallel structures:

   _plot_bars_values          :: [ (x,[y]) ],
   _plot_bars_labels          :: [[BarLabel]]

feels awkward to me. I think it would be cleaner to just include the labels directly, ie

   _plot_bars_values          :: [ (x,[(y,String)]) ],

@rdnetto
Copy link
Contributor Author

rdnetto commented May 17, 2017

I was completely unaware of the Annotations module - I've removed the code it duplicated and updated one of the tests/examples to demonstrate it.

I've modified the bar labels code as suggested, and defined a lens to preserve compatibility. It's not quite law abiding, since it always sets the labels to empty strings, but I couldn't think of a better way of handling it.

@timbod7
Copy link
Owner

timbod7 commented May 21, 2017

Thanks - I've taken a quick look, and it looks good now.

I'll review in detail in the next couple of days.

@clayrat clayrat mentioned this pull request May 31, 2023
@timbod7 timbod7 merged commit 497cb81 into timbod7:master Jun 3, 2023
@timbod7
Copy link
Owner

timbod7 commented Jun 3, 2023

Fixed by #253

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.

2 participants