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

Update to nvd3 1.8.6 #166

Merged
merged 2 commits into from
Oct 29, 2020
Merged

Update to nvd3 1.8.6 #166

merged 2 commits into from
Oct 29, 2020

Conversation

PetrDlouhy
Copy link
Contributor

@PetrDlouhy PetrDlouhy commented Oct 28, 2020

I tried to fix #165 and update dependencies. I find out, that the biggest problem is #118, which is not properly fixed by #137 or anything else, that I found (the error is fixed, but the tooltip doesn't work..

Then I realised, that the new tooltip is much better in ndv3 1.8.6 and probably solves many thinks, that are tweaked by the tooltip code in python-nvd3 and contains more information (like series color). The default tooltip works very good in most cases. The only thing, that it does not solve is to have custom date format for tooltip different than xAxisFormat.

So I tried to fix that by using header/keyFormatter functions. It works, but differently for each chart type. So the resulting code have many conditions, and should be properly tested on different chart types (I tested on my charts, but all are have date in x axis).

@PetrDlouhy
Copy link
Contributor Author

Now I found out, that with this method is possible to format date even on stackedAreaChart tooltip separately, which was not possible previously.

I also forget to write, that this probably breaks the chart.tooltip_condition_string functionality. I am not sure, what that variable actually does.

@areski
Copy link
Owner

areski commented Oct 29, 2020

It was such a long time I'm not so sure what tooltip_condition_string was used for :)
but it seems to be mainly used by piechart, anyway your PR is not breaking the output so we should be good and we could remove also tooltip_condition_string all together, it could easily be something that is not necessary with recent version.

@areski areski merged commit e6f537c into areski:develop Oct 29, 2020
@areski
Copy link
Owner

areski commented Oct 29, 2020

Ohhhh and forgot to say, huge thanks for your PR 😄

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.

Release new version
2 participants