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

Axis labels overlap if size is chosen too small #87

Closed
mrubli opened this issue Jul 2, 2020 · 12 comments
Closed

Axis labels overlap if size is chosen too small #87

mrubli opened this issue Jul 2, 2020 · 12 comments
Assignees

Comments

@mrubli
Copy link

mrubli commented Jul 2, 2020

If the size of the height graph is too small the axis labels start to overlap:

image

With the merge of #84 (also see the discussion there) this has become more obvious and requires client-side minimum sizes (see #86 for an example of how this is done).

It would be desirable to have a more dynamic approach that automatically adjusts the number of axis ticks to avoid this problem.

I'm going to take a stab at this, I just wanted to create an issue first.

@mrubli
Copy link
Author

mrubli commented Jul 2, 2020

#72 is somewhat related to this.

@mrubli
Copy link
Author

mrubli commented Jul 9, 2020

@TheGreatRefrigerator Would you mind taking a look at this proof-of-concept branch?
https://github.com/mrubli/Leaflet.Heightgraph/tree/feature/dynamic-axis-labels
This will obviously need some more refactoring and optimization but you get the idea.

The PoC implements two different algorithms that are documented inline (look for UseSimpleAlgo). There are still some visual bugs in borderline cases (very small height) but I think especially the "better" algorithm does a good job at preventing overlap in a way that looks natural.

Some questions that we'll need to address going forward:

  1. Do we need to expose different algorithms to the user or just pick one? The "better" algorithm is quite a bit more expensive and real-time resizing can already be a bit sluggish even without this logic. (To the point where, if I pull too fast, I "lose" the resize handle.)
  2. What do we do about Why are xTicks and yTicks set using Math.pow #72? I personally like the idea of just passing the tick values to d3 as-is. (It sounds like that was the plan before the commit accidentally got into the release?)
  3. Should we have a "auto" feature for xTicks/yTicks that picks a reasonable number of ticks by itself?

@TheGreatRefrigerator
Copy link
Collaborator

@mrubli i've had a quick glance at the branch, but wasn't able to test it yet. I will be on vacation next week and will take a proper look at it afterwards.

It seems to me, that both algorithms will be fine for "normal" use cases (if you don't use jquery to make it resizable by hand)
I guess most of the time the size will be defined by the app it's implemented in so i'm not sure if the sluggishness will be an issue.

Still i was thinking of a way that might improve the speed.
distance and height difference are given as static numbers, so we might be able to define a default width and height for the labels that are not exceeded by the lables in most of the cases.
You wouldn't have to iterate over every label but calculate the amount of lables that fit into the current size from the absolute distance, the resized width and the label width.

e.g. assume a default lable width of 5 digits that are let's say 50 px each. if you have a width of 300 (for the graph part, not the whole plugin, need to account for that too) there will be "place" for 6 labels and the tick count can be set accordingly.

Having such an approach it will be a "simple" calculation that might speed up the dynamic resizing by hand considerably.
I don't know if it's easy to implement, but might be worth a look at.

  1. Should we have a "auto" feature for xTicks/yTicks that picks a reasonable number of ticks by itself?

definitely. It is prefered that the heightgraph can take care of this and you don't need to trial and error until you have the right tick combinations.

2\. What do we do about #72? I personally like the idea of just passing the tick values to d3 as-is. (It sounds like that was the plan before the commit accidentally got into the release?)

I was only testing with one single example when i introduced that change. I observed that i needed to raise the x and y ticks a lot to actually see an effect. The exponential setting was making sense for the setting of the example, but not in general i realized afterwards.
I'm still not quite sure how d3 handles the tick values, or what unit it is.

The passing of the tick values should make sense in a way.
like passing 1 you have 1 tick on the axis or passing 50 you have a tick every 50 meters (or units).
But more in 2 weeks.
Cheers

@TheGreatRefrigerator
Copy link
Collaborator

I've solved it by using a solution from the original repository which derives the tick settings from height and width as well.
It works fine. Feel free to test it before the release.

@mrubli
Copy link
Author

mrubli commented Jul 30, 2020

Let me check it out. I had been thinking some more about this and was mostly worried about the case where users style the axis labels and end up using a larger font. I'm afraid that method might break down in that scenario.

@TheGreatRefrigerator
Copy link
Collaborator

Ah i see,

I still think it should be calculated by using the width and height instead of iterating over all labels.
But you could add e.g. a font factor that decides when it will switch to a low tick count, in these calculations:
f5a0df0#diff-b6d1adf805b3695396fa0405fbe17f51R5312

@mrubli
Copy link
Author

mrubli commented Jul 30, 2020

Once you scale the font and play a bit with the margins it's pretty easy to get the overlap again:
image

I also wonder how well the current solution works with high DPI displays but I don't have one to test it with. I think what we have now covers most cases pretty well but if you want to make the logic bullet-proof iterating over the labels after they're rendered may be the only solution that can get you correct results in every case.

Are you mainly worried about code complexity or performance?

@TheGreatRefrigerator
Copy link
Collaborator

I am worried about both 😬

I just don't like the idea of hiding labels conditionally after generating them.
Instead we should be using all the conditions initially when creating them.

But the problem currently is me using the whole width instead of the svg width 🤦
The handling of a scaled font can then be passed by adding a scaling factor (either seperate to x and y or simultaneously)

This could be passed as additional parameters to the heightgraph.
(When using larger fonts, reduce ticks "earlier" when "shrinking" and the other way round)
Do you know what i mean?

How exactly are you styling the fonts currently?

One additional thing reducing the overlap for the x would be #34

@TheGreatRefrigerator
Copy link
Collaborator

Can you check again with e36e130
(development branch)
If the font size still is a serious issue?

@mrubli
Copy link
Author

mrubli commented Jul 30, 2020

I am worried about both 😬

I'm more worried about the performance than the complexity. After a bit of cleanup the complexity seems manageable. Let me do some measurements on the performance to get an idea how much time it takes for the iteration method.

I just don't like the idea of hiding labels conditionally after generating them.
Instead we should be using all the conditions initially when creating them.

There's something that feels unclean about it, I agree.
The problem with the pre-conditions is that you never quite know what the browser is going to end up rendering. It may perform font replacement, it may scale things, then there's the whole issue of not using pixels as a unit when specifying font sizes, etc. Also once you start to internationalize/translate labels things get very complicated very quickly.

But the problem currently is me using the whole width instead of the svg width 🤦
The handling of a scaled font can then be passed by adding a scaling factor (either seperate to x and y or simultaneously)

This could be passed as additional parameters to the heightgraph.
(When using larger fonts, reduce ticks "earlier" when "shrinking" and the other way round)
Do you know what i mean?

I understand the idea but the main problem with passing layout-related options is that you have to mix code with style. From the perspective of a library user that's not a very desirable approach.

How exactly are you styling the fonts currently?

I just put this in the CSS:

.axis {
	font-size: 20px;
}

(In a real application I probably wouldn't use px as a unit, though.)

One additional thing reducing the overlap for the x would be #34

Actually, I like that one. It's a great way to save space and clean up the graph a bit, especially when you think about internationalization. In Chinese "20 km" would read "20公里" and having the unit after each label just looks a bit "overloaded".

@mrubli
Copy link
Author

mrubli commented Jul 30, 2020

Can you check again with e36e130
(development branch)
If the font size still is a serious issue?

That fix definitely improve things. 👍
It takes quite a bit of malice now to break it, though it's still possible:

.axis {
	font-size: 30px;
}

image

Perhaps we can leave it as is and see if people run into problems during practical use? I'll find out shortly when I get to the styling part of my own use of the library.

Either way, I'll still clean up and benchmark my original proposal, just because I'm curious and because it's nice to archive things in a clean state in case you need it one day. 😄

@TheGreatRefrigerator
Copy link
Collaborator

I would leave it like this for now too, it should cover most cases :)

Either way, I'll still clean up and benchmark my original proposal, just because I'm curious and because it's nice to archive things in a clean state in case you need it one day. 😄

Yeah! It's always sad if you put some effort into doing something and it's not used.
We might add it as an optional thing if people are running into issues.

Best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants