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

introduced xAxisLabelFormatter #302

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

Conversation

saurabhnanda
Copy link

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:

{
  xAxisLabelFormatter: function(xAxisWidth, xAxisOriginaLabels, xIsSeries) {
    // create a COPY of xAxisOriginalLabels, transform individual labels, and return a new array of labels
  }
}

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/

Screen Shot 2020-08-18 at 11 55 02 AM

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:
  • Nope.

@scmmishra
Copy link
Contributor

scmmishra commented Sep 16, 2020

Why the need for a separate formatter in charts? You can format the labels and send them in the first place?

@saurabhnanda
Copy link
Author

Why the need for a separate formatter in charts? You can format the labels and send them in the first place?

On master, can you do the following:

  • Tooltip displays the data value and data label, BUT
  • the x-axis does not display the data label, or displays the data-label differently (eg. shortened)?

@scmmishra
Copy link
Contributor

  • the x-axis does not display the data label, or displays the data-label differently (eg. shortened)?

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.

@saurabhnanda
Copy link
Author

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.

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 %d %b, %Y format?

Screen Shot 2020-08-18 at 11 55 02 AM

@scmmishra
Copy link
Contributor

scmmishra commented Sep 18, 2020

I don't think this is possible.

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?
Copy link
Contributor

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;
Copy link
Contributor

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?

Copy link
Contributor

@scmmishra scmmishra left a 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!

@scmmishra
Copy link
Contributor

@saurabhnanda let me know when the changes are done

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.

2 participants