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

More options for sparkline + bug fix #60

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

fceruti
Copy link

@fceruti fceruti commented May 7, 2022

I really wanted to add some custom styling for my project, so I modified accordingly.

Here's an image of both the custom styling and the zero filled data list bug resolved.

Let me know if more work is needed to merge this, I’ll be happy to do it.

Screen Shot 2022-05-07 at 19 13 24

line_stroke_linecap: line_stroke_linecap,
line_stroke_linejoin: line_stroke_linejoin
}) do
~s|stroke="#{line_stroke}" class="#{line_class}" stroke-width="#{line_stroke_width}" fill="#{line_fill}" stroke-linecap="#{line_stroke_linecap}" stroke-linejoin="#{line_stroke_linejoin}" vector-effect="non-scaling-stroke" |
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't looked at the later commits yet, but it may be better to eliminate nil values. Contex.SVG.opts_to_attrs may help.

@mindok
Copy link
Owner

mindok commented May 9, 2022

Looks good! I can see quite a few improvements coming to the other chart types along similar lines. Thanks, also, for fixing up some of the variable naming etc. There are probably three jobs to do to finalise this:

  • Update changelog.md
  • Check for breaking changes (it doesn't look like there are any)
  • Update the sample app to show it off

@fceruti
Copy link
Author

fceruti commented May 12, 2022

Thanks :) I'll have a little bit of time this weekend to wrap this up.

@fceruti
Copy link
Author

fceruti commented Jun 12, 2022

Sorry for the late response, life got in the way :P

I've been thinking about this PR, and I've had second thoughts about the proposed API.

As of now, I've proposed a flat API, but maybe a nested one makes more sense and also opens the possibility for other parts of the API to take a similar approach. My main concern concern is to help establish an idiom for the library that's global, understandable & extendable (I've got some more charts in the oven).

Please let me know your thoughts on this:

@default_style [
  height: 20,
  width: 100,
  extra_svg: nil,
  line: [
    stroke: "rgba(0, 200, 50, 0.7)",
    class: nil,
    stroke_width: 1,
    stroke_linecap: "round",
    stroke_linejoin: "round",
    fill: "none"
  ],
  area: [
    stroke: "none",
    fill: "rgba(0, 200, 50, 0.2)",
    class: nil
  ]
]

@mindok
Copy link
Owner

mindok commented Jun 20, 2022

Apologies - life got in the way here too! We had high winds on 11th June that caused quite a lot of damage in our area so we have been busy with cleanup.

I agree with your thinking - a nested structure looks easier to navigate. While the sparklines are a special case in Contex, consistent handling of styling would make it a lot better developer experience.

It does get a bit more difficult for the other chart types as each series may have different styling, and over time it would be good to set different styling attributes by different variables in the dataset.

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