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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix/20166-columns-gridLines-and-pointPlacement #20414

Closed

Conversation

MarkusBarstad
Copy link
Contributor

@MarkusBarstad MarkusBarstad commented Jan 10, 2024

Fixed #20166, columns with pointPlacement had a superfluous tick to the left of the chart, visible with gridLineWidth.

Note: Firefox headless broke at me so this was committed with --no-verify so I could review github-side checks 馃槵

This could be a messy addition to otherwise clean code but I am unsure if i should:

  • Create a variable at the top of the axis.render function which checks for column series with pointPlacement
  • Create it closer to the check
  • Handle it some other way

@MarkusBarstad MarkusBarstad added Product: Highcharts Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog. labels Jan 10, 2024
@MarkusBarstad MarkusBarstad self-assigned this Jan 10, 2024
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Jan 10, 2024

File size comparison

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

master candidate difference
highcharts.js 96.0 kB
271.3 kB
96.0 kB
271.4 kB
23 B
47 B
highstock.js 128.7 kB
372.7 kB
128.8 kB
372.7 kB
24 B
47 B
highmaps.js 121.5 kB
349.3 kB
121.5 kB
349.4 kB
39 B
95 B
highcharts-gantt.js 131.0 kB
377.5 kB
131.0 kB
377.6 kB
24 B
47 B
modules/heatmap.js 7.2 kB
19.3 kB
7.2 kB
19.3 kB
11 B
48 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Jan 10, 2024

Visual test results - Differences found

Found 1 diffing sample(s). Please review the differences.

@bre1470
Copy link
Contributor

bre1470 commented Jan 11, 2024

Good news: It works here with Firefox ESR (v115).

Copy link
Contributor

@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.

Looks good so far. This fixes the second issue example.

Things that should be improved:

  • broaden the scope of the fix
  • a visual test is needed, this properly can be part of an existing sample

if (
!axis.series.find( // #20166
(series): string | number | false | undefined => (
series.is('column') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You should fix this for all cartesian series types. Instead of series.is(... check series.xAxis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

It now checks for series.xAxis and unit-test has been altered to check for this as well.

Might not have come out clearly in the issue but this is the only example which I was able to replicate both in jsfiddle as well as utils.

I have however uncovered a similar issue for the bar series, but it does seem to arise from the same conditions (tick[-1] being rendered for cartesian series using pointPlacement)

Should I create a seperate issue for the bar series bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

The bar series is cartesian, but it has inverted axis. This needs taken into account and is probably related.
See https://api.highcharts.com/highcharts/chart.inverted

@TorsteinHonsi
Copy link
Collaborator

Instead of looking for series with the pointPlacement option, I think we should just check that the tick position is inside the axis. That would be simpler and safer.

For the grid line, we have a force option for the getPlotLinePath function. For some reason it is set to pass for the grid lines. If we comment out that forde parameters, the problematic grid line is not rendered. So I think we should find out why and in what cases we want to force it to render.

For the tick mark we don't seem to have that kind of overflow protection, but we should probably have something similar. Anything that is not between axis.pos and axis.pos + axis.length should not be rendered.

@bre1470
Copy link
Contributor

bre1470 commented Jan 15, 2024

When running npx gulp test --browsercount 1 there are now 11 instead of 6 (master) tests failing.

@MarkusBarstad
Copy link
Contributor Author

MarkusBarstad commented Jan 15, 2024

When running npx gulp test --browsercount 1 there are now 11 instead of 6 (master) tests failing.

Yeah you are right, I think gulp "got stale" or something and did not actually track my changes, allowing a failing commit.

@hubertkozik
Copy link
Member

The work will be continued in this PR: #20467 Due to many no-verify commits here, which can cause issues with future bisecting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Use on PR to add description as a bugfix in the generated changelog. Product: Highcharts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plot-background and x/y axis groups are misaligned for column series
5 participants