Skip to content

H17: Create chart with csv data#13

Open
dqmrf wants to merge 3 commits into
mainfrom
highcharts/17-data-module
Open

H17: Create chart with csv data#13
dqmrf wants to merge 3 commits into
mainfrom
highcharts/17-data-module

Conversation

@dqmrf

@dqmrf dqmrf commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@dqmrf dqmrf requested a review from raf18seb June 30, 2026 11:04

@raf18seb raf18seb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi! Almost perfect, only 2 request changes - the first one here, the second one in the comments.

  1. The tooltip in the columnrange series is not showing the hours, see the screenshots
Image

Not a mistake, but a small additional comment: try to avoid setting the constant max and tickInterval - letting Highcharts to calculate it automatically is usually a better choice.

max: 24 * 60,
tickInterval: 60,

Comment on lines +55 to +59
labels: {
formatter() {
return this.value + '°' + T_UNITS[currentTUnit];
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Whenever you can, use format instead of formatter (better e.g. for JSON configs)
labels: {
    format: `{value}°${T_UNITS[currentTUnit]}`
},

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Format will not work here because when you press the button: currentTUnit variable changes and axis text must change as well.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Then perhaps you should update the yAxis.labels.format on a button click as well? Only a few lines of additional code, but it'll work for more specific cases.

You don't have to change it now, but please keep it in mind - format is usually better than formatter. We had many users on support with problems when the formatter wasn't working in specific cases, e.g. when the chart config had to be a JSON string (no functions, only strings).
Soon you'll work with Highcharts core code, and build API and documentation, and you'll have to use format only.

ps. I can see the .attr() on the yAxis text - using yAxis.update({ title: { text: ... } }) is also better because you store all real values on the chart (with .attr, you only change DOM)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Okay. I'll keep that in mind.

@dqmrf dqmrf requested a review from raf18seb July 2, 2026 06:45

@raf18seb raf18seb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, good job 👏

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