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

fix: always get points so state last works with other graphs #1075

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

Conversation

jasonlewis
Copy link

Fixes #736 by always getting points.

There's probably a few ways to fix this as it seems the state: last functionality specifically relies on points being available, but points are only set when specifically showing points (and never for bar graphs).

I decided to always define points and adjust the logic for rendering points to be based on whether the config is set. Granted I haven't tested all scenarios for rendering points so this may not be an adequete solution in all cases.

src/main.js Show resolved Hide resolved
@akloeckner
Copy link
Collaborator

@jasonlewis, thanks for contributing! It seems we'll have to work on it a bit still. See my comment. Unfortunately, I will only be able to look into it in a few days...

@akloeckner
Copy link
Collaborator

I did think about this quite some time. Sorry for the delay.

Although it's a small change, I don't like that we would treat points differently from the other properties, such as bar. So, I wondered, could we make this more consistent and came up with two ideas:

  1. What do you think about always defining all those properties and moving the conditions to the render* functions? I'm a bit hesitant, because I still don't fully understand, how much calculating this would cause.

  2. One other thing, I just stumbled against is: we could maybe just use this.Graph[id].coords[-1][V] instead of this.points[id][-1][V]. Or would we abuse the Graph then?

What do you think? Maybe you discarded these ideas already for some reason?

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.

None yet

2 participants