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

Implement feature request #4237 plus refactor similar scale multi-axis functions #4247

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

rosco54
Copy link
Contributor

@rosco54 rosco54 commented Feb 18, 2024

New feature

Implement feature request #4237
See new: sample/src/area/multi-axis-with-seriesname-arrays.xml

Note: it is left to the creativity of the user to visually disambiguate which plot is associated with which Y axis.

Extended application of this feature

The current style of mapping multiple series to a y axis is to include one yaxis config per series but set each yaxis seriesName to the same series name and then set show: false on all axes that are not required.

While not the purpose of #4237, the feature offers a more intuitive alternative that explicitly configures yaxis elements with the series that will be referenced to them (seriesName: []). This only requires including the yaxis elements that will be seen on the chart. This is how it works to the best of my limited knowledge.

Old way:
yax: ser
0: 0
1: 1
2: 1
3: 1
4: 1

Axes 0..4 are all scaled and all are be rendered unless the axes are show: false. If the chart is stacked, it's assumed that series 1..4 corresponding to the yaxis indices 1..4 are the series' that will be stacked.

Implementation of the extended feature

Use the new feature yaxis: seriesName: [] to simplify the specification of series inclusion in stacked charts, especially when combined with other series. Essentially, refactor Scales.sameScaleInMultipleAxes() and Scales.setMultipleYScales() to use yaxis: seriesName: [].

New way:
yax: ser
0: [0]
1: [1,2,3,4]

See stacked-column-with-line.xml
and c.f. stacked-column-with-line-new.xml

The new facility is backward compatible with the existing style via code that converts the old data structure to the new one.

If the chart is stacked (a global chart setting), it is assumed that any axis with multiple series is stacked. Multiple yaxes referencing multiple series in a stacked chart is not implemented (or rather doesn't do anything particularly useful yet), however it was noted that it does not break the chart. It essentially does what a user might expect it to do, which is add an additional Y axis.

Fix stacked chart y axis scaling: min and max were computed as the sum of the series min and max, not the min and max of the one-for-one summed datapoints in each series.

Minor fixes

Additional checks for undefined elements in CoreUtils.extendArrayProps().

Remove the duplicate outlier in boxplot-scatter.xml.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@rosco54
Copy link
Contributor Author

rosco54 commented Feb 19, 2024

Please hold off reviewing. There is an issue that I need to address.

@rosco54 rosco54 force-pushed the Issue_4237 branch 4 times, most recently from 8a2a26a to 05e1e37 Compare February 19, 2024 02:08
@rosco54
Copy link
Contributor Author

rosco54 commented Feb 19, 2024

The issue (an off-by-one mistake) has now been fixed, plus added some unit tests. Should be good to review. Thanks.

@rosco54 rosco54 marked this pull request as draft February 19, 2024 02:54
@rosco54 rosco54 marked this pull request as ready for review February 19, 2024 04:08
area/multi-axis-with-seriesname-arrays.xml

- also use this facility to simplify the
specification of series inclusion in stacked charts, especially when
combined with other series.

See stacked-column-with-line.xml
and c.f. stacked-column-with-line-new.xml

The new facility should be backward compatible with the existing
style (both described below to the best of my knowledge), via code
that converts the old data structure to the new one.

The current style of mapping multiple series to a y axis is to
include one yaxis config per series but set each yaxis seriesName to
the same series name and then set show: false on all axes that are not
required.

While not the purpose of apexcharts#4237, it offers a more intuitive alternative
that explicitly configures yaxis elements with the series that will be
referenced to them (seriesName: []). This only requires including the
yaxis elements that will be seen on the chart.

Old way:
  yax:	ser
  0:	0
  1:	1
  2:	1
  3:	1
  4:	1

Axes 0..4 are all scaled and all will be rendered unless the axes are
show: false. If the chart is stacked, it's assumed that series 1..4 are
the contributing series.

New way:
  yax:	ser
  0:	[0]
  1:	[1,2,3,4]

If the chart is stacked (a global chart setting), it must be assumed
that any axis with multiple series is stacked, presumably as separate
sets (not yet implemented).
 

Fix stacked chart y axis scaling: min and max were computed as the
sum of the series min and max, not the min and max od the summed
datapoints in each series.

Note: it is left to the creativity of the user to visually disambiguate
which plot is associated with which Y axis.

Additional checks for undefined elements in CoreUtils.extendArrayProps().

Remove the duplicate outlier in boxplot-scatter.xml.
@junedchhipa junedchhipa merged commit 9c256c5 into apexcharts:main Feb 23, 2024
@rosco54 rosco54 deleted the Issue_4237 branch February 29, 2024 07:06
j2ghz added a commit to j2ghz/apexcharts.js that referenced this pull request Apr 29, 2024
@j2ghz j2ghz mentioned this pull request Apr 29, 2024
10 tasks
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