chartist icon indicating copy to clipboard operation
chartist copied to clipboard

Use two different containers for horizontal and vertical lines

Open mweimerskirch opened this issue 8 years ago • 10 comments

This makes styling the grid much more flexible, because it allows css selectors such as ":first-child", ":last-child", ":nth-child(even)", etc to be used.

mweimerskirch avatar Jun 27 '16 09:06 mweimerskirch

Hi there. Thanks for the PR! Please fix the failing tests before we process this any further. I guess this is not really a breaking change, although it will effect current stylings in some projects I guess. We should ship this with a minor not with a patch I guess.

gionkunz avatar Jun 27 '16 11:06 gionkunz

I fixed the tests. And yes, I agree that this this should not be in 0.9.x, but rather in 0.10 or 1.0. But that choice is up to you =)

mweimerskirch avatar Jun 27 '16 13:06 mweimerskirch

I made the same change for the labels as I had the same "issue" with the labels as well (not being able to properly apply alternate styles using "nth-child(even)" and the like.

mweimerskirch avatar Jun 27 '16 16:06 mweimerskirch

The question now is, do we still need the .ct-vertical and .ct-horizontal classes? Since we could rely on child selectors now right. If this a breaking change anyway, we should flag it for 1.0.0 and even remove the .ct-vertical and .ct-horizontal classes. But then we also need to re-write our Sass files.

gionkunz avatar Jun 27 '16 17:06 gionkunz

I've used the classes in the "chart.on('draw', ...)" callback to distinguish which element I'm currently modifying. That might be a reason to keep them, but with an additional line of code I could also read the class from the parent element ;-) Other people might rely on those classes for the styling. I don't really see a reason why they need to be removed. It's not that this would hurt performance.

mweimerskirch avatar Jun 27 '16 20:06 mweimerskirch

Hi @mweimerskirch Can you please resolve the conflicts with the recently added grid background functionality by @hansmaad ? After this I think we're good to go! Sorry for the delay here.

gionkunz avatar Jul 26 '16 07:07 gionkunz

I actually don't see any value being added here.

You can already select first/last grid lines with the -of-type selectors.

.ct-grid.ct-horizontal:first-of-type: First horizontal grid line .ct-grid.ct-vertical:last-of-type: Last vertical grid line

There's even :nth-of-type() to target a specific index of horizontal/vertical. This PR only adds more dom node levels, more logic and would break users' existing css for grid lines.

JonDum avatar Aug 19 '16 21:08 JonDum

@JonDum No, first-of-type or nth-of-type only matches the element name, not the class. So if you the following structure:

<div class="ct-vertical"></div>
<div class="ct-vertical"></div>
<div class="ct-horizontal"></div>
<div class="ct-horizontal"></div>

... there is no way to match the first element with that class. ":nth-match" will be able to do that, but that's not supported by any browser yet, as it's still part of the CSS4 draft.

mweimerskirch avatar Oct 24 '16 16:10 mweimerskirch

@gionkunz I rebased the code and adapted it for the grid backgrounds. I placed the grid background element in the same container as the horizontal lines. Not sure if that is the best way though.

mweimerskirch avatar Oct 24 '16 16:10 mweimerskirch

:shipit: but maybe retain emitting the ct-grids and ct-labels css classes for backwards compatibly?

e.g. emit BOTH classes so existing stuff that styles via ct-grids still works

IDisposable avatar Dec 15 '16 16:12 IDisposable