nullscreen / squid

A Ruby library to plot charts in PDF files
http://nullscreen.github.io/squid/
MIT License
220 stars 42 forks source link

Add `:col_max_length` setting #51

Open grantneufeld opened 8 years ago

grantneufeld commented 8 years ago

Allows for a maximum column width to be specified for charts of type :column or :stack.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling f20ffd38553feb4c9caad28614b47b3e519fe868 on grantneufeld:master into bd2e988f838d2930afe168af566bb7c548b21056 on Fullscreen:master.

claudiofullscreen commented 8 years ago

Hello @grantneufeld and thanks for stopping by and for using Squid!

Can you help me understand what is a use-case for this new setting?

Thanks!

grantneufeld commented 8 years ago

I am generating a chart that needs to fill a set space on the page, and only has a couple of columns. But, the columns are ending up very wide, so I needed a way to make them thinner without shrinking the width of the chart. I’m attaching an example image for comparison. chart-column-width-comparisson

grantneufeld commented 8 years ago

However, before you accept this pull request, I’m thinking now that there may be a better way to define the column max width setting — especially if Squid is to ever offer other options for modifying columns (gradient fills, picture fills, borders, shadows, etc.). Perhaps something like a :col_settings setting, which would take a hash, for which :max_width would be one option?

So, instead of chart data, :col_max_width there would be chart data, col_settings: { max_width: 123, … }

claudiofullscreen commented 8 years ago

I see. Thanks for your reply!

I never thought of having options that are specific to a column. I try to keep the public API of squid as slim as possible.

If you are thinking of moving in that direction, maybe you can try different attempts in your local branch / fork until you find one that's completely satisfying for you.

At that point, you might update your PR and it will be easier for me to get a full overview of the changes required to the API compared with the advantages of the new settings.

Thanks again!

grantneufeld commented 8 years ago

Will do.

grantneufeld commented 8 years ago

I’ve completely rethought the approach I took in this pull request. Instead of adding the very limited and specific :col_max_length configuration option, I’ve implemented an approach for passing in an object to define behavior (as inspired by Sandi Metz 😊). This “renderer” object (I’m totally open to other names), allows for users to do anything they want to the PDF in place of the normal rendering.

So far, I’ve only implemented custom “renderer” support for plain (not stacked) columns, and the little boxes in the legend.

You can see the code I’ve put together for this (including working examples/squid/ files for the manual) at: https://github.com/grantneufeld/squid/commit/d05b9160a2a3270ba75a162711f92e555fc4ac89 (Although the examples are, admittedly, not particularly exciting, they do illustrate how columns can be made to look quite different, while still matching the appropriate dimensions.)

I’ve tried to, through the examples, provide detailed documentation for using the renderer objects.

Before I try another pull request here, please let me know what you think of this whole idea, and what improvements/changes could make it more suited for inclusion in Squid.

Thanks!

claudiofullscreen commented 8 years ago

@grantneufeld wow, you went all the way with your research! Good job!! 👏 🎀

If I understand correctly, you are thinking of splitting the work of Squid into a base part (that calculates where the points will be plotted) and a rendered (which plots those point as graphical elements).

I think the idea is good. In my code, I call "plotter" what you call "renderer".

As you write in your branch, this opens some questions regarding configuration, objects, code complexity, which makes me think we would still need some work on top of your branch to have this all sketched out.

For the current version, I don't have the time to delve into a re-organization of the API to make room for that. I'm happy to follow your progress in your local branch and see where that brings you, and eventually consider this new structure for the next major version of the gem.

Thanks again 🎀

grantneufeld commented 8 years ago

Yes, splitting the drawing/plotting/rendering behaviour from the position/scale/calculating behaviour should make customizing/changing the chart formatting quite flexible and easier to extend.

I’m not sure when I’ll get another block of time to continue this work — I hope soon, but can’t make a specific commitment, as my workload is a bit full.

grantneufeld commented 8 years ago

I did a little more work on this, implementing support for customizing the axis labels/ticks: https://github.com/grantneufeld/squid/commit/8ca2dc40bc9691d8bb0b2cf125ea11ad73ae1178

Slowly getting there… 😊