-
Notifications
You must be signed in to change notification settings - Fork 722
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
introduced xAxisLabelFormatter #302
base: master
Are you sure you want to change the base?
Conversation
Why the need for a separate formatter in charts? You can format the labels and send them in the first place? |
On
|
Yes it is technically possible. You can provide as little information as you want with the data labels by passing it as it is, and add all the verbosity using the Tooltip formatter. The implementation aside, I don't see a good enough reason for this to be a feature yet. If you could provide more use cases for this, that warrants this to be a first class feature, I'd love to hear them. |
I don't think this is possible. You want the x-axis to have LESS information (and in certain cases the label should be omitted complete), while the tooltip has MORE information. Would you be able to implement a graph (for the purpose of a counter-example) as shown below where (a) the x-axis displays the full date only for month boundaries, else it displays only the day portion, and (b) the tooltip displays the date in |
As I said, it's technically possible (Not from charts out of the box), the toolTip formatter can infer the full data from a different source instead of charts. Here is a sandbox example of this. I understand that a library should provide all customization options and let the developers decide if they want to use it or not. However, I have to maintain the sanity of the code while doing so, that is the reason I vet all features that show up in my inbox, hope you understand. If you can present a good enough use case for this, I would definitely merge this, which was my point in the first place. Anyway, I think we can merge this feature, it's not a large one that would bloat the code. P.S. if you wish to show lesser information on the Y-Axis in general, You could use the xIsSeries option for contiguous data. |
@@ -1,3 +1,15 @@ | |||
# What's in this fork? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this portion of README from the PR?
@@ -39,6 +39,7 @@ export default class AxisChart extends BaseChart { | |||
this.config.yAxisMode = options.axisOptions.yAxisMode || 'span'; | |||
this.config.xIsSeries = options.axisOptions.xIsSeries || 0; | |||
this.config.shortenYAxisNumbers = options.axisOptions.shortenYAxisNumbers || 0; | |||
this.config.xAxisLabelFormatter = options.xAxisLabelFormatter || getShortenedLabels; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this is axisOptions
instead of the top level option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the dist/*
and docs/*
build from the PR. It creates conflicts for no reason.
I'll be cleaning this repository mess soon with v2, thanks!
@saurabhnanda let me know when the changes are done |
Explanation About What Code Achieves:
Frappe Charts doesn't give you a way to format x-axis labels. This is especially required when you have a lot of data points in a series and the Frappe Charts ends up truncating x-axis labels (by default). You end-up with a mess in the x-axis consisting of tons of ellipsis - essentially making the x-axes useless. This fork introduces the following config parameter:
This allows you to transform/modify the x-axis labels for the the x-axis and for the tooltip independently of each other
Screenshots/GIFs:
Here's a jsfiddle with this new config in action - https://jsfiddle.net/05mv4wat/2/
Steps To Test:
Take a look at the jsfiddle. I have tested only for line/axis charts. Before merging please test this doesn't break other chart-types.
TODOs: