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 :col_max_length setting #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add :col_max_length setting #51

wants to merge 1 commit into from

Conversation

grantneufeld
Copy link

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

Allows for a maximum column width to be specified for charts of type `:column` or `:stack`.
@coveralls
Copy link

Coverage Status

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

@claudiofullscreen
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

grantneufeld commented Apr 15, 2016

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
Copy link
Contributor

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
Copy link
Author

Will do.

@grantneufeld
Copy link
Author

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: grantneufeld@d05b916
(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
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Author

I did a little more work on this, implementing support for customizing the axis labels/ticks:
grantneufeld@8ca2dc4

Slowly getting there… 😊

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.

3 participants