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

demo/pnf-series-move-to-demo #21114

Merged
merged 8 commits into from
Jun 10, 2024
Merged

Conversation

kamil-musialowski
Copy link
Contributor

Added new demo category called Experimental, moved and improved Point and figure series.

@highsoft-bot
Copy link
Collaborator

File size comparison

No differences found

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented May 3, 2024

Visual test results - No difference found


Samples changed

Change type Sample
Modified samples/stock/demo/pointandfigure/demo.js
Modified samples/stock/demo/pointandfigure/demo.js
Modified samples/stock/demo/pointandfigure/demo.js
Modified samples/stock/demo/pointandfigure/demo.js
Modified samples/stock/demo/pointandfigure/demo.js
Modified samples/stock/demo/pointandfigure/demo.js
? samples/highcharts/studies/pointandfigure/demo.js

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.

In addition to the comment below:

  • can we adjust the space between columns of points? I mean - decrease the space?

samples/stock/demo/pointandfigure/demo.js 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.

As discussed:

  • let's go back to columnMetric solutions
  • set max width for the shape, to avoid too flat shapes

samples/stock/demo/pointandfigure/demo.js Outdated Show resolved Hide resolved
samples/stock/demo/pointandfigure/demo.js 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.

One last thing, promise! :D

When you zoom in close enough, it throws errors in the console - could you simply fix it using if clause? By close enough I mean no visible points

Note:
There are also some problems with panning, but for now, let's ignore them

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.

Thanks!

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, it looks good! I have some comments on the structure of the code itself:

  • What we are demonstrating is a plugin. So I think the demo should have a clearer separation between the plugin and the actual demo (the chart config and generation). The plugin itself doesn't have to be inside the async IIFE. But it can be in an IIFE so that it is clearly encapsulated. This is a good pattern:
(({ isNumber, relativeLength, defined, pick }) => {
    // Plugin code here
})(Highcharts);
  • We should have some more comments. Most importantly, a comment for the plugin itself, stating that this is a complicated demo showing an experimental feature of Highcharts Stock, but also serving as a demonstration for how to extend the library. The point is that we don't want readers to see this code and believe that it is complicated to set up Highcharts Stock. The plugin may look intimidating, so we need to explain what it is.
  • Now the async IIFE should contain only the chart configuration itself.

samples/stock/demo/pointandfigure/demo.js Outdated Show resolved Hide resolved
@pawelfus
Copy link
Contributor

pawelfus commented Jun 5, 2024

Is there anything else to add here @TorsteinHonsi ? Or is it ready to merge?

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.

Looks good, thanks 👍

@TorsteinHonsi TorsteinHonsi merged commit 69ad6ec into master Jun 10, 2024
13 checks passed
@TorsteinHonsi TorsteinHonsi deleted the demo/pnf-series-move-to-demo branch June 10, 2024 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants