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

Feature/zoom in polar #16992

Merged
merged 53 commits into from
Oct 27, 2022
Merged

Feature/zoom in polar #16992

merged 53 commits into from
Oct 27, 2022

Conversation

bm64
Copy link
Member

@bm64 bm64 commented Feb 14, 2022

Added new feature, zooming on polar charts. See #16992.

To do:

@bm64 bm64 added the Changelog: Feature Use on PR to add description as a feature in the generated changelog. label Feb 14, 2022
@bm64 bm64 self-assigned this Feb 14, 2022
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Feb 14, 2022

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
highcharts.js 99.7 kB
295.7 kB
99.9 kB
296.4 kB
201 B
696 B
highstock.js 132.6 kB
400.7 kB
132.8 kB
401.4 kB
187 B
690 B
highmaps.js 125.2 kB
380.3 kB
125.4 kB
381.0 kB
273 B
692 B
highcharts-gantt.js 135.3 kB
405.3 kB
135.5 kB
405.9 kB
191 B
692 B
modules/annotations.js 17.9 kB
63.7 kB
17.9 kB
63.7 kB
4 B
11 B
modules/annotations-advanced.js 24.7 kB
96.4 kB
24.7 kB
96.4 kB
4 B
11 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Feb 14, 2022

Visual test results - No difference found


Samples changed

Change type Sample
Added samples/highcharts/chart/zoomtype-polar/demo.js

@bm64 bm64 marked this pull request as ready for review August 31, 2022 09:13
Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the inline comments, I am not sure we need two separate methods (and events) for createSelectionMarker and getSelectionMarkerAttrs.

The thing is that the selection marker can either a rect, path or arc. And the attributes are always SVGAttributes. All of these objects can be created without initial attributes, we we can probably just create one singe extendable function that returns the element type and the attributes. Much like point.shapeType and point.shapeArgs.

            // make a selection
            const selectionMarkerShape = this.getSelectionMarker(chartX, chartY);
            if (
                (chart.hasCartesianSeries || chart.mapView) &&
                (this.zoomX || this.zoomY) &&
                clickedInside &&
                !panKey
            ) {
                if (!selectionMarker) {
                    this.selectionMarker = selectionMarker = chart.renderer[selectionMarkerShape.type]()
                        .attr({
                            'class': 'highcharts-selection-marker',
                            zIndex: 7
                        })
                        .add();

                    if (!chart.styledMode) {
                        selectionMarker.attr({
                            fill:
                                chartOptions.selectionMarkerFill ||
                                color(Palette.highlightColor80)
                                    .setOpacity(0.25).get()
                        });
                    }
                }
            }

            if (selectionMarker) {
                selectionMarker.attr(
                    selectionMarkerShape.attribs
                );
            }

ts/Core/Pointer.ts Outdated Show resolved Hide resolved
ts/Core/Pointer.ts Outdated Show resolved Hide resolved
ts/Core/Pointer.ts Outdated Show resolved Hide resolved
@bm64
Copy link
Member Author

bm64 commented Sep 26, 2022

Hello @EWChina999! Sorry for longer response time and thanks for sharing the idea. This ticket is strictly related to polar zoom, so if you don’t mind, please create a separated feature request so we can carry the discussion there: https://github.com/highcharts/highcharts/issues/new/choose

Copy link
Collaborator

@TorsteinHonsi TorsteinHonsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Some more comments. Trying to reduce the filesize impact on highcharts.js.

* @private
* @function Highcharts.Point#determineIsNull
*/
public determineIsNull(): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logic is there because the isValid function is not part of the basic Point object, just some extended objects. So instead of wrapping it, I think we should just implement the function on the basic Point.

public isValid(): boolean {
   return this.x !== null && isNumber(this.y);
}

ts/Core/Chart/Chart.ts Outdated Show resolved Hide resolved
ts/Core/Pointer.ts Outdated Show resolved Hide resolved
ts/Core/Pointer.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@pawelfus pawelfus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@pawelfus pawelfus requested a review from bre1470 October 7, 2022 13:07
@pawelfus
Copy link
Contributor

pawelfus commented Oct 7, 2022

@bre1470 could you review too? I see a few as any castings, but I don't have any great ideas on how to solve them

@bre1470
Copy link
Member

bre1470 commented Oct 7, 2022

Yes, I will check. In the meantime please merge the latest master into this branch, so that CI jobs get updated.

Copy link
Member

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! 👍 See the comments for any replacements.

ts/Core/Axis/RadialAxis.ts Outdated Show resolved Hide resolved
ts/Series/PolarComposition.ts Outdated Show resolved Hide resolved
ts/Series/PolarComposition.ts Outdated Show resolved Hide resolved
ts/Series/PolarComposition.ts Outdated Show resolved Hide resolved
@bm64 bm64 requested review from bre1470 and TorsteinHonsi October 11, 2022 21:49
Copy link
Member

@bre1470 bre1470 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good. 👍

@TorsteinHonsi TorsteinHonsi merged commit 02e9e7f into master Oct 27, 2022
@highsoft-bot highsoft-bot added this to the Next milestone Oct 27, 2022
@TorsteinHonsi TorsteinHonsi deleted the feature/zoom-in-polar branch October 27, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Feature Use on PR to add description as a feature in the generated changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants